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

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:56
+bool isComparisonOperator(OverloadedOperatorKind Kind) {
+  return llvm::is_contained(
+      std::initializer_list<OverloadedOperatorKind>{
----------------
5chmidti wrote:
> 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.
If the optimiser is able to see through the original set up then there's no issue and the initial code should be fine.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/SymmetricBinaryOperatorCheck.cpp:84
+
+static Expected<DynTypedNode> getNode(const BoundNodes &Nodes, StringRef ID) {
+  auto &NodesMap = Nodes.getMap();
----------------
5chmidti wrote:
> 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.
In which case make the below an assert and return by DynTypedNode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128861