[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator

[Julian Schmidt via Phabricator via cfe-commits]

5chmidti added a comment.

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.



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:56
+bool isComparisonOperator(OverloadedOperatorKind Kind) {
+  return llvm::is_contained(
+      std::initializer_list<OverloadedOperatorKind>{
----------------
njames93 wrote:
> Could these not be represented as a bitset?
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.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:84
+
+static Expected<DynTypedNode> getNode(const BoundNodes &Nodes, StringRef ID) {
+  auto &NodesMap = Nodes.getMap();
----------------
njames93 wrote:
> Should there ever be a case where this returns an error. If not can this assert instead and just return a DynTypedNode?
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.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:135-136
+    };
+    return invalidArgumentError(
+        "constSpec invalid argument: not a CXXMethodDecl");
+  };
----------------
njames93 wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128861/new/

https://reviews.llvm.org/D128861