Skip to content

Commit 98146c1

Browse files
poelmancnjames93
authored andcommitted
[clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons
`clang-tidy -std=c++20` with `modernize-use-nullptr` mistakenly inserts `nullptr` in place of the comparison operator if the comparison internally expands in the AST to a rewritten spaceship operator. This can be reproduced by running the new `modernize-use-nullptr-cxx20.cpp` test without applying the supplied patch to `UseNullptrCheck.cpp`; the current clang-tidy will mistakenly replace: ```result = (a1 < a2);``` with ```result = (a1 nullptr a2);``` Reviewed By: njames93 Differential Revision: https://reviews.llvm.org/D95714
1 parent 068bf9e commit 98146c1

File tree

2 files changed

+59
-4
lines changed

2 files changed

+59
-4
lines changed

clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,28 @@ StatementMatcher makeCastSequenceMatcher() {
4141
unless(hasImplicitDestinationType(qualType(substTemplateTypeParmType()))),
4242
unless(hasSourceExpression(hasType(sugaredNullptrType()))));
4343

44+
auto IsOrHasDescendant = [](auto InnerMatcher) {
45+
return anyOf(InnerMatcher, hasDescendant(InnerMatcher));
46+
};
47+
4448
return traverse(
4549
TK_AsIs,
46-
castExpr(anyOf(ImplicitCastToNull,
47-
explicitCastExpr(hasDescendant(ImplicitCastToNull))),
48-
unless(hasAncestor(explicitCastExpr())))
49-
.bind(CastSequence));
50+
anyOf(castExpr(anyOf(ImplicitCastToNull,
51+
explicitCastExpr(hasDescendant(ImplicitCastToNull))),
52+
unless(hasAncestor(explicitCastExpr())),
53+
unless(hasAncestor(cxxRewrittenBinaryOperator())))
54+
.bind(CastSequence),
55+
cxxRewrittenBinaryOperator(
56+
// Match rewritten operators, but verify (in the check method)
57+
// that if an implicit cast is found, it is not from another
58+
// nested rewritten operator.
59+
expr().bind("matchBinopOperands"),
60+
hasEitherOperand(IsOrHasDescendant(
61+
implicitCastExpr(
62+
ImplicitCastToNull,
63+
hasAncestor(cxxRewrittenBinaryOperator().bind(
64+
"checkBinopOperands")))
65+
.bind(CastSequence))))));
5066
}
5167

5268
bool isReplaceableRange(SourceLocation StartLoc, SourceLocation EndLoc,
@@ -480,6 +496,11 @@ void UseNullptrCheck::check(const MatchFinder::MatchResult &Result) {
480496
const auto *NullCast = Result.Nodes.getNodeAs<CastExpr>(CastSequence);
481497
assert(NullCast && "Bad Callback. No node provided");
482498

499+
if (Result.Nodes.getNodeAs<CXXRewrittenBinaryOperator>(
500+
"matchBinopOperands") !=
501+
Result.Nodes.getNodeAs<CXXRewrittenBinaryOperator>("checkBinopOperands"))
502+
return;
503+
483504
// Given an implicit null-ptr cast or an explicit cast with an implicit
484505
// null-to-pointer cast within use CastSequenceVisitor to identify sequences
485506
// of explicit casts that can be converted into 'nullptr'.
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// RUN: %check_clang_tidy -std=c++20 %s modernize-use-nullptr %t
2+
3+
namespace std {
4+
struct strong_ordering {
5+
int n;
6+
constexpr operator int() const { return n; }
7+
static const strong_ordering equal, greater, less;
8+
};
9+
constexpr strong_ordering strong_ordering::equal = {0};
10+
constexpr strong_ordering strong_ordering::greater = {1};
11+
constexpr strong_ordering strong_ordering::less = {-1};
12+
} // namespace std
13+
14+
class A {
15+
public:
16+
auto operator<=>(const A &other) const = default;
17+
};
18+
19+
void test_cxx_rewritten_binary_ops() {
20+
A a1, a2;
21+
bool result;
22+
// should not change next line to (a1 nullptr a2)
23+
result = (a1 < a2);
24+
// CHECK-FIXES: result = (a1 < a2);
25+
// should not change next line to (a1 nullptr a2)
26+
result = (a1 >= a2);
27+
// CHECK-FIXES: result = (a1 >= a2);
28+
int *ptr = 0;
29+
// CHECK-FIXES: int *ptr = nullptr;
30+
result = (a1 > (ptr == 0 ? a1 : a2));
31+
// CHECK-FIXES: result = (a1 > (ptr == nullptr ? a1 : a2));
32+
result = (a1 > ((a1 > (ptr == 0 ? a1 : a2)) ? a1 : a2));
33+
// CHECK-FIXES: result = (a1 > ((a1 > (ptr == nullptr ? a1 : a2)) ? a1 : a2));
34+
}

0 commit comments

Comments
 (0)