[PATCH] D128861: [clang-tidy] add cppcoreguidelines-symmetric-binary-operator
Julian Schmidt via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Aug 20 14:35:30 PDT 2022
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
More information about the cfe-commits
mailing list