poelmanc created this revision.
poelmanc added reviewers: aaron.ballman, angelgarcia, hokein.
poelmanc added a project: clang-tools-extra.
Herald added a subscriber: yaxunl.
poelmanc requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
As in https://reviews.llvm.org/D95714, `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);
Oops!
The supplied patch fixes this by adding just one comment and one line of code
to UseNullptrCheck.cpp:
// Skip implicit casts to 0 generated by operator<=> rewrites.
unless(hasGrandparent(cxxRewrittenBinaryOperator())),
It also adds the new `hasGrandparent` matcher, which is necessary as opposed to
`hasAncestor` to properly convert:
result = (a1 > (ptr == 0 ? a1 : a2));
to
result = (a1 > (ptr == nullptr ? a1 : a2));
In this case the AST has an "ancestor" that is a rewritten binary operator, but
not a direct grandparent.
This is an alternative to https://reviews.llvm.org/D95714.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D95726
Files:
clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/modernize-use-nullptr-cxx20.cpp
clang/include/clang/ASTMatchers/ASTMatchers.h
clang/include/clang/ASTMatchers/ASTMatchersInternal.h
clang/lib/ASTMatchers/ASTMatchFinder.cpp
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===================================================================
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -659,6 +659,8 @@
ResultCache.clear();
if (MatchMode == AncestorMatchMode::AMM_ParentOnly)
return matchesParentOf(Node, Matcher, Builder);
+ else if (MatchMode == AncestorMatchMode::AMM_GrandparentOnly)
+ return matchesGrandparentOf(Node, Matcher, Builder);
return matchesAnyAncestorOf(Node, Ctx, Matcher, Builder);
}
@@ -893,6 +895,18 @@
return false;
}
+ // Returns whether a direct grandparent of \p Node matches \p Matcher.
+ bool matchesGrandparentOf(const DynTypedNode &Node,
+ const DynTypedMatcher &Matcher,
+ BoundNodesTreeBuilder *Builder) {
+ for (const auto &Parent : ActiveASTContext->getParents(Node)) {
+ if (matchesParentOf(Parent, Matcher, Builder)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
// Returns whether an ancestor of \p Node matches \p Matcher.
//
// The order of matching (which can lead to different nodes being bound in
Index: clang/include/clang/ASTMatchers/ASTMatchersInternal.h
===================================================================
--- clang/include/clang/ASTMatchers/ASTMatchersInternal.h
+++ clang/include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -654,7 +654,10 @@
AMM_All,
/// Direct parent only.
- AMM_ParentOnly
+ AMM_ParentOnly,
+
+ /// Direct grandparent only.
+ AMM_GrandparentOnly
};
virtual ~ASTMatchFinder() = default;
@@ -1654,6 +1657,29 @@
}
};
+/// Matches nodes of type \c T that have a grandparent node of type
+/// \c GrandparentT for which the given inner matcher matches.
+///
+/// \c GrandparentT must be an AST base type.
+template <typename T, typename GrandparentT>
+class HasGrandparentMatcher : public MatcherInterface<T> {
+ static_assert(IsBaseType<GrandparentT>::value,
+ "has parent only accepts base type matcher");
+
+ const DynTypedMatcher GrandparentMatcher;
+
+public:
+ explicit HasGrandparentMatcher(
+ const Matcher<GrandparentT> &GrandparentMatcher)
+ : GrandparentMatcher(GrandparentMatcher) {}
+
+ bool matches(const T &Node, ASTMatchFinder *Finder,
+ BoundNodesTreeBuilder *Builder) const override {
+ return Finder->matchesAncestorOf(Node, this->GrandparentMatcher, Builder,
+ ASTMatchFinder::AMM_GrandparentOnly);
+ }
+};
+
/// Matches nodes of type \c T that have at least one ancestor node of
/// type \c AncestorT for which the given inner matcher matches.
///
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===================================================================
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -3434,6 +3434,22 @@
internal::TypeList<Decl, NestedNameSpecifierLoc, Stmt, TypeLoc>>
hasParent;
+/// Matches AST nodes that have a grandparent that matches the provided
+/// matcher.
+///
+/// Given
+/// \code
+/// void f() { for (;;) { int x = 42; if (true) { int x = 43; } } }
+/// \endcode
+/// \c compoundStmt(hasGrandparent(forStmt())) matches "{ int x = 43; }".
+///
+/// Usable as: Any Matcher
+extern const internal::ArgumentAdaptingMatcherFunc<
+ internal::HasGrandparentMatcher,
+ internal::TypeList<Decl, NestedNameSpecifierLoc, Stmt, TypeLoc>,
+ internal::TypeList<Decl, NestedNameSpecifierLoc, Stmt, TypeLoc>>
+ hasGrandparent;
+
/// Matches AST nodes that have an ancestor that matches the provided
/// matcher.
///
Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-nullptr-cxx20.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-nullptr-cxx20.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy -std=c++20 %s modernize-use-nullptr %t
+
+#include <compare>
+
+class A {
+public:
+ auto operator<=>(const A &other) const = default;
+};
+
+void test_cxx_rewritten_binary_ops() {
+ A a1, a2;
+ bool result;
+ // should not change next line to (a1 nullptr a2)
+ result = (a1 < a2);
+ // CHECK-FIXES: result = (a1 < a2);
+ // should not change next line to (a1 nullptr a2)
+ result = (a1 >= a2);
+ // CHECK-FIXES: result = (a1 >= a2);
+ int *ptr = 0;
+ // CHECK-FIXES: int *ptr = nullptr;
+ result = (a1 > (ptr == 0 ? a1 : a2));
+ // CHECK-FIXES: result = (a1 > (ptr == nullptr ? a1 : a2));
+}
Index: clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
@@ -45,6 +45,8 @@
TK_AsIs,
castExpr(anyOf(ImplicitCastToNull,
explicitCastExpr(hasDescendant(ImplicitCastToNull))),
+ // Skip implicit casts to 0 generated by operator<=> rewrites.
+ unless(hasGrandparent(cxxRewrittenBinaryOperator())),
unless(hasAncestor(explicitCastExpr())))
.bind(CastSequence));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
