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