[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 320393.
poelmanc added a comment.

Thanks @steveire, that suggestion worked perfectly! I added the additional test case and shortened the mimicked `strong_ordering` code to a version from `clang/unittests.ASTMatchers/ASTMatchersTraversalTest.cpp`, but also manually tested this using both MSVC's and libstdc++v3's `<compare>` header.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95714/new/

https://reviews.llvm.org/D95714

Files:
  clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-use-nullptr-cxx20.cpp


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,34 @@
+// RUN: %check_clang_tidy -std=c++20 %s modernize-use-nullptr %t
+
+namespace std {
+struct strong_ordering {
+  int n;
+  constexpr operator int() const { return n; }
+  static const strong_ordering equal, greater, less;
+};
+constexpr strong_ordering strong_ordering::equal = {0};
+constexpr strong_ordering strong_ordering::greater = {1};
+constexpr strong_ordering strong_ordering::less = {-1};
+}
+
+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));
+  result = (a1 > ((a1 > (ptr == 0 ? a1 : a2)) ? a1 : a2));
+  // CHECK-FIXES: result = (a1 > ((a1 > (ptr == nullptr ? a1 : a2)) ? 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
@@ -41,12 +41,28 @@
       unless(hasImplicitDestinationType(qualType(substTemplateTypeParmType()))),
       unless(hasSourceExpression(hasType(sugaredNullptrType()))));
 
+  auto isOrHasDescendant = [](auto InnerMatcher) {
+    return anyOf(InnerMatcher, hasDescendant(InnerMatcher));
+  };
+
   return traverse(
       TK_AsIs,
-      castExpr(anyOf(ImplicitCastToNull,
-                     explicitCastExpr(hasDescendant(ImplicitCastToNull))),
-               unless(hasAncestor(explicitCastExpr())))
-          .bind(CastSequence));
+      anyOf(castExpr(anyOf(ImplicitCastToNull,
+                           explicitCastExpr(hasDescendant(ImplicitCastToNull))),
+                     unless(hasAncestor(explicitCastExpr())),
+                     unless(hasAncestor(cxxRewrittenBinaryOperator())))
+                .bind(CastSequence),
+            cxxRewrittenBinaryOperator(
+                // Match rewritten operators, but verify (in the check method)
+                // that if an implicit cast is found, it is not from another
+                // nested rewritten operator
+                expr().bind("matchBinopOperands"),
+                hasEitherOperand(isOrHasDescendant(
+                    implicitCastExpr(
+                        ImplicitCastToNull,
+                        hasAncestor(cxxRewrittenBinaryOperator().bind(
+                            "checkBinopOperands")))
+                        .bind(CastSequence))))));
 }
 
 bool isReplaceableRange(SourceLocation StartLoc, SourceLocation EndLoc,
@@ -480,6 +496,11 @@
   const auto *NullCast = Result.Nodes.getNodeAs<CastExpr>(CastSequence);
   assert(NullCast && "Bad Callback. No node provided");
 
+  if (Result.Nodes.getNodeAs<CXXRewrittenBinaryOperator>(
+          "matchBinopOperands") !=
+      Result.Nodes.getNodeAs<CXXRewrittenBinaryOperator>("checkBinopOperands"))
+    return;
+
   // Given an implicit null-ptr cast or an explicit cast with an implicit
   // null-to-pointer cast within use CastSequenceVisitor to identify sequences
   // of explicit casts that can be converted into 'nullptr'.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D95714.320393.patch
Type: text/x-patch
Size: 3702 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210201/4901c653/attachment-0001.bin>