[Clang-tidy] Fix for bug 31838: readability-delete-null-pointer does not work for class members

[madsravn]

Summary

Made a small fix to readability-delete-null-pointer check such that it includes class members.

[alexfh]

clang-tidy/readability/DeleteNullPointerCheck.cpp

46 ↗

(On Diff #87688)

nit: Will has(anyOf(DeleteExpr, DeleteMemberExpr)) work?

test/clang-tidy/readability-delete-null-pointer.cpp

69 ↗

(On Diff #87688)

nit: I'd put this line closer to the line with the warning (or right before it), e.g.

// CHECK-MESSAGES: :[[@LINE+1]]:...
if (mp)
  delete mp;

71 ↗

(On Diff #87688)

This doesn't check that the if is deleted. It will pass if the check doesn't do any replacements. One way to properly check this is to add a unique comment after the if and match it with an anchor to the start of line:

if (mp) // should be deleted 1
  delete mp;
// CHECK-FIXES: {{^   }}// should be deleted 1
// CHECK-FIXES-NEXT: delete mp;

or alternatively:

// next line should be deleted
if (mp)
  delete mp;
// CHECK-FIXES: // next line should be deleted
// CHECK-FIXES-NEXT:  {{^  }}delete mp;

[madsravn]

Small change to check.
Minor changes to the lit test.

[madsravn]

clang-tidy/readability/DeleteNullPointerCheck.cpp

46 ↗

(On Diff #87688)

I could swear that I had tried that. It seems like the obvious way to do it. It worked, though.

[alexfh]

test/clang-tidy/readability-delete-null-pointer.cpp

67 ↗

(On Diff #87829)

This should match the comment to verify the if is deleted.

[madsravn]

Added comment in CHECK-FIX to ensure the line we are referring to is uniquely identified.

[alexfh]

LG

clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.h

 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_DELETE_NULL_POINTER_H
 
 #include "../ClangTidy.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
-/// Check whether the 'if' statement is unnecessary before calling 'delete' on a pointer.
+/// Check whether the 'if' statement is unnecessary before calling 'delete' on a
+/// pointer.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/readability-delete-null-pointer.html
 class DeleteNullPointerCheck : public ClangTidyCheck {
 public:
   DeleteNullPointerCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_DELETE_NULL_POINTER_H

clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.cpp

 namespace readability {
 
 void DeleteNullPointerCheck::registerMatchers(MatchFinder *Finder) {
   const auto DeleteExpr =
       cxxDeleteExpr(has(castExpr(has(declRefExpr(
                         to(decl(equalsBoundNode("deletedPointer"))))))))
           .bind("deleteExpr");
 
-  const auto PointerExpr =
-      ignoringImpCasts(declRefExpr(to(decl().bind("deletedPointer"))));
+  const auto DeleteMemberExpr =
+      cxxDeleteExpr(has(castExpr(has(memberExpr(hasDeclaration(
+                        fieldDecl(equalsBoundNode("deletedMemberPointer"))))))))
+          .bind("deleteMemberExpr");
+
+  const auto PointerExpr = ignoringImpCasts(anyOf(
+      declRefExpr(to(decl().bind("deletedPointer"))),
+      memberExpr(hasDeclaration(fieldDecl().bind("deletedMemberPointer")))));
+
   const auto PointerCondition = castExpr(hasCastKind(CK_PointerToBoolean),
                                          hasSourceExpression(PointerExpr));
   const auto BinaryPointerCheckCondition =
       binaryOperator(hasEitherOperand(castExpr(hasCastKind(CK_NullToPointer))),
                      hasEitherOperand(PointerExpr));
 
   Finder->addMatcher(
       ifStmt(hasCondition(anyOf(PointerCondition, BinaryPointerCheckCondition)),
-             hasThen(anyOf(DeleteExpr,
-                           compoundStmt(has(DeleteExpr), statementCountIs(1))
-                               .bind("compound"))))
+             hasThen(anyOf(
+                 DeleteExpr, DeleteMemberExpr,
+                 compoundStmt(has(anyOf(DeleteExpr, DeleteMemberExpr)),
+                              statementCountIs(1))
           .bind("ifWithDelete"),
       this);
 }
 
 void DeleteNullPointerCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *IfWithDelete = Result.Nodes.getNodeAs<IfStmt>("ifWithDelete");
   const auto *Compound = Result.Nodes.getNodeAs<CompoundStmt>("compound");
 

clang-tools-extra/trunk/test/clang-tidy/readability-delete-null-pointer.cpp

void f() {
   if (c2) {
     // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'if' statement is unnecessary;
     // CHECK-FIXES: } else {
     // CHECK-FIXES: c2 = c;
     delete c2;
   } else {
     c2 = c;
   }
+  struct A {
+    void foo() {
+      if (mp) // #6
+        delete mp;
+      // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: 'if' statement is unnecessary; deleting null pointer has no effect [readability-delete-null-pointer]
+      // CHECK-FIXES: {{^      }}// #6
+      // CHECK-FIXES-NEXT: delete mp;
+    }
+    int *mp;
+  };
 }
 
 void g() {
   int *p5, *p6;
   if (p5)
     delete p6;
 
   if (p5 && p6)
     delete p5;
 
   if (p6) {
     int x = 5;
     delete p6;
   }
 }