[clang-tools-extra] cebb7c0 - [clang-tidy] modernize-use-nullptr matches "NULL" in templates (#109169)

[via cfe-commits]

Author: Thomas Köppe
Date: 2024-09-25T09:25:46-07:00
New Revision: cebb7c010854e39a77065cfd681db91a79e7ce15

URL: https://github.com/llvm/llvm-project/commit/cebb7c010854e39a77065cfd681db91a79e7ce15
DIFF: https://github.com/llvm/llvm-project/commit/cebb7c010854e39a77065cfd681db91a79e7ce15.diff

LOG: [clang-tidy] modernize-use-nullptr matches "NULL" in templates (#109169)

Make modernize-use-nullptr matcher also match "NULL", but not "0", when
it appears on a substituted type of a template specialization.

Previously, any matches on a substituted type were excluded, but this
meant that a situation like the following is not diagnosed:

```c++
template <typename T>
struct X {
  T val;
  X() { val = NULL; }  // should diagnose
};
```

When the user says `NULL`, we expect that the destination type is always
meant to be a pointer type, so this should be converted to `nullptr`. By
contrast, we do not propose changing a literal `0` in that case, which
appears as initializers of both pointer and integer specializations in
reasonable real code. (If `NULL` is used erroneously in such a
situation, it should be changed to `0` or `{}`.)

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
    clang-tools-extra/docs/ReleaseNotes.rst
    clang-tools-extra/test/clang-tidy/checkers/modernize/use-nullptr.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
index 6a003a347badac..108717e151b577 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
@@ -35,10 +35,20 @@ AST_MATCHER(Type, sugaredNullptrType) {
 /// to null within.
 /// Finding sequences of explicit casts is necessary so that an entire sequence
 /// can be replaced instead of just the inner-most implicit cast.
+///
+/// TODO/NOTE: The second "anyOf" below discards matches on a substituted type,
+/// since we don't know if that would _always_ be a pointer type for all other
+/// specializations, unless the expression was "__null", in which case we assume
+/// that all specializations are expected to be for pointer types. Ideally this
+/// would check for the "NULL" macro instead, but that'd be harder to express.
+/// In practice, "NULL" is often defined as "__null", and this is a useful
+/// condition.
 StatementMatcher makeCastSequenceMatcher(llvm::ArrayRef<StringRef> NameList) {
   auto ImplicitCastToNull = implicitCastExpr(
       anyOf(hasCastKind(CK_NullToPointer), hasCastKind(CK_NullToMemberPointer)),
-      unless(hasImplicitDestinationType(qualType(substTemplateTypeParmType()))),
+      anyOf(hasSourceExpression(gnuNullExpr()),
+            unless(hasImplicitDestinationType(
+                qualType(substTemplateTypeParmType())))),
       unless(hasSourceExpression(hasType(sugaredNullptrType()))),
       unless(hasImplicitDestinationType(
           qualType(matchers::matchesAnyListedTypeName(NameList)))));

diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 44b1f8c07edd3c..9a130a23b6e896 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -166,6 +166,10 @@ Changes in existing checks
   a false positive when only an implicit conversion happened inside an
   initializer list.
 
+- Improved :doc:`modernize-use-nullptr
+  <clang-tidy/checks/modernize/use-nullptr>` check to also recognize
+  ``NULL``/``__null`` (but not ``0``) when used with a templated type.
+
 - Improved :doc:`modernize-use-std-print
   <clang-tidy/checks/modernize/use-std-print>` check to support replacing
   member function calls too.

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-nullptr.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-nullptr.cpp
index 7bc0925136aa86..2c36349da896cf 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-nullptr.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-nullptr.cpp
@@ -84,6 +84,31 @@ void test_macro_expansion4() {
 #undef MY_NULL
 }
 
+template <typename T> struct pear {
+  // If you say __null (or NULL), we assume that T will always be a pointer
+  // type, so we suggest replacing it with nullptr. (We only check __null here,
+  // because in this test NULL is defined as 0, but real library implementations
+  // it is often defined as __null and the check will catch it.)
+  void f() { x = __null; }
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: use nullptr [modernize-use-nullptr]
+  // CHECK-FIXES: x = nullptr;
+
+  // But if you say 0, we allow the possibility that T can be used with integral
+  // and pointer types, and "0" is an acceptable initializer (even if "{}" might
+  // be even better).
+  void g() { y = 0; }
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]] warning: use nullptr
+
+  T x;
+  T y;
+};
+void test_templated() {
+  pear<int*> p;
+  p.f();
+  p.g();
+  dummy(p.x);
+}
+
 #define IS_EQ(x, y) if (x != y) return;
 void test_macro_args() {
   int i = 0;