Adds a check for C++ Core Guidelines "C.161: Use non-member functions for symmetric operators"
Produces a warning on member operators that are symmetric binary operators
and generates fixes for symmetric defaulted comparison operators to be changed
to friend operator functions.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Please note that documentation and test locations were changed recently. Please rebase from main and make necessary changes.
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.h | ||
---|---|---|
28 | Please add isLanguageVersionSupported. | |
clang-tools-extra/docs/ReleaseNotes.rst | ||
105 | Please keep alphabetic order of entries. | |
108 | One sentence should be enough. | |
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-symmetric-binary-operator.rst | ||
6 ↗ | (On Diff #441186) | Please make first sentence same as in Release Notes. |
10 ↗ | (On Diff #441186) | This should be at end of documentation. |
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.h | ||
---|---|---|
30 | This method is usually inlined. |
Question: Should this check be extended to also include C.86: "Flag an operator==() for which the argument types differ; same for other comparison operators: !=, <, <=, >, and >=."? With a warning along the lines of: "comparison operators should be symmetric with respect to the parameters types" or "parameters of comparison operators should be of the same type" (or some other warning). Or should that be a new check? I think it fits.
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp | ||
---|---|---|
25–35 | Just bring the whole namespace into scope. | |
57 | Could these not be represented as a bitset? | |
85 | Should there ever be a case where this returns an error. If not can this assert instead and just return a DynTypedNode? | |
118–123 | Looks like this is begging to be a while loop. | |
130–134 | If its unreachable, use unreachable to indicate that. | |
135 | This semi-colon looks suspicious | |
136–137 | Is this error reachable? Again if not use unreachable. |
I've got everything done locally, but I'm waiting on the decision on the bitset to update the diff, including some minor additional updates I will list then.
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp | ||
---|---|---|
57 | Yes. I changed this to use a bitset. While changing it I thought about the performance and came to these two solutions: bool isComparisonOperator(OverloadedOperatorKind Kind) { std::bitset<NUM_OVERLOADED_OPERATORS> BitSetCompOps{}; BitSetCompOps.set(OO_Less); BitSetCompOps.set(OO_Greater); BitSetCompOps.set(OO_EqualEqual); BitSetCompOps.set(OO_ExclaimEqual); BitSetCompOps.set(OO_LessEqual); BitSetCompOps.set(OO_GreaterEqual); BitSetCompOps.set(OO_Spaceship); BitSetCompOps.set(OO_AmpAmp); BitSetCompOps.set(OO_PipePipe); return BitSetCompOps[Kind]; } B) the same, but this emits much less code during -O0 (14 instructions) and for -O1 and up it's the same as A) template <OverloadedOperatorKind... ComparisonKinds> constexpr size_t isComparisonOperatorGenerateBitset() { return ((static_cast<size_t>(1U) << ComparisonKinds) | ...); } bool isComparisonOperator(OverloadedOperatorKind Kind) { constexpr static auto BitsetCompOps = isComparisonOperatorGenerateBitset< OO_Less, OO_Greater, OO_EqualEqual, OO_ExclaimEqual, OO_LessEqual, OO_GreaterEqual, OO_Spaceship, OO_AmpAmp, OO_PipePipe>(); return BitsetCompOps & (static_cast<size_t>(1U) << Kind); } See also https://godbolt.org/z/E9aeW67xb for both examples. | |
85 | I got this bit from the Transformer lib implementation, but you're right, there are no paths in this check that call getNode with an unbound ID. | |
136–137 | Yes it's unreachable, constSpec get's only called on bound node "op" which is a CXXMethodDecl. Changed it to llvm_unreachable like the other unreachable. |
- address all comments
- minor renames of variables/functions
- add missing & when accessing OptTokens value
- rebase onto current HEAD
- fix llvm::Optional member deprecation warnings
- add support to match when the parameter type explicitly instantiaties itself (i.e. const A<T>& instead of const A& for template <typename T> struct A {...};)
lower c++ version requirement by making the fixes conditional on c++20 instead of the whole check
Because there was no activity on this patch, I am closing it. I should've pinged a bit more :)
With the move to GitHub pull requests soon, pushing this patch further in phabricator does not make sense to me. Instead, I could reopen this patch as a pull request. There are some problems I have encountered anyway that need fixing.
Is there general interest in this check?
The cppcoreguidelines prefix could be changed to something else (e.g. modernize), if that is a hindrance.
For me check is fine and bring some value. But for me auto-fixes in this check aren't needed, this simply complicate a check that could be very simple.
You might be right. I will fix the issues I have found and will propose this check without the fixes. For completeness, I will mention that I have the implementation for them available, and raise your concern about the complexity of introducing these fixes.
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp | ||
---|---|---|
29 | again, I would probably try to generate simple switch with those macros., and this function should be static. | |
48 | For me this function is overkill, I would use simple switch and let compiler optimize it. except that should be static. | |
62–68 | put matchers into anonymous namespace.... | |
164 | Consider writing this check as an normal simple one without using TransformerClangTidyCheck as base. After that half of this code won't be needed, and it will be very easy to maintain/fix/extend it. |
Thanks for your review comments :) . I'll incorporate them and reference your comments when I post the patch.
Please add isLanguageVersionSupported.