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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
