This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] add cppcoreguidelines-symmetric-binary-operator
AbandonedPublic

Authored by 5chmidti on Jun 29 2022, 3:04 PM.

Details

Summary

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.

Diff Detail

Event Timeline

5chmidti created this revision.Jun 29 2022, 3:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 3:04 PM
5chmidti requested review of this revision.Jun 29 2022, 3:04 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript
5chmidti updated this revision to Diff 441185.Jun 29 2022, 3:07 PM

rm unused function

5chmidti updated this revision to Diff 441186.Jun 29 2022, 3:08 PM

rm wrong op from isComparisonOperator

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
139

Please keep alphabetic order of entries.

142

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.

5chmidti updated this revision to Diff 441193.Jun 29 2022, 3:59 PM

Addressed comments

5chmidti updated this revision to Diff 441194.Jun 29 2022, 4:02 PM

Fixup addressing comments, missed changing the links for docs

5chmidti updated this revision to Diff 441196.Jun 29 2022, 4:05 PM

Fixup for the same reason

Eugene.Zelenko added inline comments.Jun 29 2022, 4:05 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.h
29

This method is usually inlined.

5chmidti updated this revision to Diff 441197.Jun 29 2022, 4:07 PM

Address inlining isLanguageVersionSupported

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.

njames93 added inline comments.Aug 17 2022, 4:14 AM
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.
If it is reachable, is it possible to show a test case where this error is expected and detected in the test?

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:
A) is straight forward but suffers from bad-ish code gen when doing -O0 (58 instructions), on -O1 and up it's just 4 instructions.

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.
I think B) would be better, but I thought I'd ask before going with a parameter pack instead of the std::bitset.

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.

njames93 added inline comments.Aug 21 2022, 12:00 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp
57

If the optimiser is able to see through the original set up then there's no issue and the initial code should be fine.

85

In which case make the below an assert and return by DynTypedNode.

5chmidti updated this revision to Diff 454287.Aug 21 2022, 2:38 AM
  • 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 {...};)
5chmidti updated this revision to Diff 454292.Aug 21 2022, 3:35 AM

lower c++ version requirement by making the fixes conditional on c++20 instead of the whole check

5chmidti marked 13 inline comments as done.Aug 21 2022, 3:41 AM
LegalizeAdulthood resigned from this revision.Mar 29 2023, 8:46 AM
5chmidti abandoned this revision.Aug 21 2023, 10:23 AM

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.

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.

PiotrZSL added inline comments.Aug 21 2023, 11:07 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp
30

again, I would probably try to generate simple switch with those macros., and this function should be static.

49

For me this function is overkill, I would use simple switch and let compiler optimize it. except that should be static.

63–69

put matchers into anonymous namespace....

165

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.