Page MenuHomePhabricator

[clang-tidy] cppcoreguidelines-explicit-constructor-and-conversion: new alias
Needs ReviewPublic

Authored by mgartmann on May 19 2021, 7:47 AM.

Details

Summary

Since the google-explicit-constructor check implements rules C.46 and C.164 from the CppCoreGuidelines, alias cppcoreguidelines-explicit-constructor-and-conversion was created.

C.46 proposes excluding any non-explicit single-argument constructors on a positive list. Such a whitelist option was added to google-explicit-constructor check.

Diff Detail

Event Timeline

mgartmann created this revision.May 19 2021, 7:47 AM
mgartmann requested review of this revision.May 19 2021, 7:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2021, 7:47 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mgartmann edited the summary of this revision. (Show Details)May 19 2021, 7:52 AM
mgartmann edited the summary of this revision. (Show Details)

Added ConstructorWhitelist option to the google-explicit-constructor check.

Dear reviewers,

Since this work was conducted as part of a Bachelor's thesis, which has to be
handed in on the 18th of June, 2021, we wanted to add a friendly ping to this
patch.

It would make us, the thesis team, proud to state that some of our work was
accepted and included into the LLVM project.

Therefore, we wanted to ask if we can further improve anything in this patch
for it to get a LGTM?

Thanks a lot in advance!
Yours faithfully,
Fabian & Marco

aaron.ballman added inline comments.May 24 2021, 11:57 AM
clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
22
23–24

It's unfortunate that this is parsing the list of names each time -- I think we should parse the string list one time and pass a container in to this function rather than doing the split every time we encounter a constructor.

114

Unintended formatting change?

clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.h
29

The community has recently been switching away from "whitelist" and "blacklist" terminology, so I'd recommend changing this to IgnoredConstructors.

Should conversion operators also have an allow list? (The core guideline doesn't mention it, but I'm wondering about code that wants an implicit conversion in a particular case and how they should silence the diagnostic in that case.)

clang-tools-extra/test/clang-tidy/checkers/google-explicit-constructor-constructorwhitelist-option.cpp
11 ↗(On Diff #346640)

It looks like there are no tests for conversion operators -- those should be added.

mgartmann updated this revision to Diff 347970.May 26 2021, 8:25 AM
mgartmann marked 5 inline comments as done.
  • added option to ignore conversion operators
  • added tests for new and existing options
  • renamed options to Ignore...
  • ensured that option's strings only get parsed once

@aaron.ballman Thanks a lot for your valuable feedback! I incorporated it accordingly.

Is there anything else that should be improved?

clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
114

This formatting change was done by clang-format when git clang-format HEAD~1 was run. Therefore, I assumed that it is now correctly formatted.

Am I wrong?

aaron.ballman added inline comments.May 26 2021, 10:14 AM
clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
28–30

Rather than trying to do this by hand, does Decl::print() give you the functionality you need? For example, this will likely not work well for classes defined within a namespace (it may ignore the wrong class due to not checking the namespace). Another thing to consider are templates and how to handle those. e.g.,

struct Foo {
  template <typename Ty>
  operator Ty() const; // How to silence the diagnostic here?
};

Thankfully, specializations can't differ in their explicitness, so you don't have to also worry about:

struct Foo {
  template <typename Ty>
  explicit operator Ty() const; // This one's explicit
};

template <>
Foo::operator int() const; // Thankfully, this inherits the explicit from the primary template.
33
114

I've never used the integrated git clang-format before, but I sort of wonder if it is getting extra context lines and that's why this one changed? I usually do git diff -U0 --no-color HEAD^ | clang-format-diff.py -i -p1 and then I know exactly what range of lines are being touched.

clang-tools-extra/docs/clang-tidy/checks/google-explicit-constructor.rst
74–75

We should probably document other identifiers that contribute to the name of the operator, like namespaces, template ids, etc. Using code examples for anything you think is tricky would be helpful.

clang-tools-extra/test/clang-tidy/checkers/google-explicit-constructor-ignoredconversionoperators-option.cpp
85

Can you also add an example that an explicit conversion operator does not diagnose?

mgartmann marked an inline comment as done.
  • added testcase of explicit operator

Is it worth using regex to ignore some operators:
*::operator bool to ignore all bool conversions etc.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-explicit-constructor-and-conversion.rst
13–15

The page generated from here will automatically redirect to the alias after 5 seconds so any extra documentation in here won't be read.

clang-tools-extra/docs/clang-tidy/checks/google-explicit-constructor.rst
75

double quotes aren't needed in options.

clang-tools-extra/test/clang-tidy/checkers/google-explicit-constructor-ignoredconversionoperators-option.cpp
2–5

Passing an explicitly empty config is unnecessary.

mgartmann updated this revision to Diff 351838.Jun 14 2021, 5:40 AM
mgartmann marked 5 inline comments as done.
  • removed empty configs from tests
  • moved documentation to Google's check
  • extended matchers so that namespaces for classes and conversion operators can be specified
  • adjusted documentation and tests

Unfortunately, I was not able to implement functionality to ignore constructors and operates by using Regex in the given time.

mgartmann added inline comments.Jun 14 2021, 5:47 AM
clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp
28–30

Thanks for your comment, @aaron.ballman!

I was able to use printQualifiedName to get a Node's qualified name, including its namespace.

Templated operators are not represented by their name visible in the source code (e.g., Ty) in the AST. Instead, their name in the AST is something like type-parameter-0-0. As it is now, templated operators have to be ignored with the latter name, which is also displayed in the check's diagnostic message.
This was described in the documentation accordingly.

I was not able to find a feasible way to enable users to exclude templated operators by their original name (e.g., Ty). Does anyone have an idea how this could be done?

mgartmann marked an inline comment as done.Jun 14 2021, 6:34 AM