[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 320330.
poelmanc added a comment.

Thanks to the great feedback, changed 
`unless(hasAncestor(cxxRewrittenBinaryOperator()))` to 
`unless(hasParent(expr(hasParent(cxxRewrittenBinaryOperator()))))` and added a 
test to verify the improvement (and removed an extraneous comment.)


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,50 @@
+// RUN: %check_clang_tidy -std=c++20 %s modernize-use-nullptr %t
+
+namespace std {
+
+struct strong_ordering {
+  static const strong_ordering less;
+  static const strong_ordering equal;
+  static const strong_ordering greater;
+  using zero_type = decltype(nullptr);
+
+  friend constexpr bool operator<(const strong_ordering value, zero_type) 
noexcept {
+    return value.comparison_value < 0;
+  }
+
+  friend constexpr bool operator>(const strong_ordering value, zero_type) 
noexcept {
+    return value.comparison_value > 0;
+  }
+
+  friend constexpr bool operator>=(const strong_ordering value, zero_type) 
noexcept {
+    return value.comparison_value >= 0;
+  }
+
+  signed char comparison_value;
+};
+
+inline constexpr strong_ordering strong_ordering::less{-1};
+inline constexpr strong_ordering strong_ordering::equal{0};
+inline constexpr strong_ordering strong_ordering::greater{1};
+
+} // namespace std
+
+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(hasParent(expr(hasParent(cxxRewrittenBinaryOperator())))),
                unless(hasAncestor(explicitCastExpr())))
           .bind(CastSequence));
 }



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,50 @@
+// RUN: %check_clang_tidy -std=c++20 %s modernize-use-nullptr %t
+
+namespace std {
+
+struct strong_ordering {
+  static const strong_ordering less;
+  static const strong_ordering equal;
+  static const strong_ordering greater;
+  using zero_type = decltype(nullptr);
+
+  friend constexpr bool operator<(const strong_ordering value, zero_type) noexcept {
+    return value.comparison_value < 0;
+  }
+
+  friend constexpr bool operator>(const strong_ordering value, zero_type) noexcept {
+    return value.comparison_value > 0;
+  }
+
+  friend constexpr bool operator>=(const strong_ordering value, zero_type) noexcept {
+    return value.comparison_value >= 0;
+  }
+
+  signed char comparison_value;
+};
+
+inline constexpr strong_ordering strong_ordering::less{-1};
+inline constexpr strong_ordering strong_ordering::equal{0};
+inline constexpr strong_ordering strong_ordering::greater{1};
+
+} // namespace std
+
+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(hasParent(expr(hasParent(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