[Clang-tidy] delete null check

[• SilverGeri]

Summary

removes the unnecessary if statements, if it was used to check a pointer's validity just to call delete on that pointer

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

I think misc-delete-null-pointer will be better name.

[Eugene.Zelenko]

Will be good idea to change some if statements in regression test to (p != nullptr) (for C++11) and (p != NULL) (pre-C+11). See http://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-cast.html.

[etienneb]

Some comments after a quick look to the code.

clang-tidy/misc/DeleteNullCheck.cpp

22 ↗

(On Diff #60549)

HasDeleteExpr -> DeleteExpr

23 ↗

(On Diff #60549)

The use of hasDescendant is Expensive.
There is cast to ignore? You could probably just skip cast/parens.

If the intend of this match-statement is to match comparison against NULL, it should be expanded to be more precise.

47 ↗

(On Diff #60549)

This check could be moved to the matcher part above/
see ASTMatcher: hasCastKind

48 ↗

(On Diff #60549)

see: equalsBoundNode

49 ↗

(On Diff #60549)

This check should be moved to the matcher part too.
see: statementCountIs

[aaron.ballman]

clang-tidy/misc/DeleteNullCheck.cpp

23 ↗

(On Diff #60549)

I think you want to use has() instead of hasDescendant() here. Otherwise, I think this code may trigger on delete foo(some_declref) where foo() returns a pointer. You may also need to ignore implicit casts here.

35 ↗

(On Diff #60549)

Won't this trigger on code like if (foo && bar) delete foo->bar;? It doesn't look like you handle that sort of case below.

[Eugene.Zelenko]

docs/clang-tidy/checks/misc-delete-null-pointer.rst

10 ↗

(On Diff #76803)

Please indent variable declaration.

[alexfh]

I guess, "readability" will be a better category for this check.

[• SilverGeri]

checks if (p != 0) conditions, too

[aaron.ballman]

clang-tidy/misc/DeleteNullPointerCheck.cpp

54 ↗

(On Diff #77015)

Rather than a parenthetical, I would just use a semicolon to distinguish the clauses. Also, I would quote 'if' to make it clear that you're talking about syntax rather than an English term.

clang-tidy/misc/DeleteNullPointerCheck.h

19 ↗

(On Diff #77015)

You should write that short description.

[Prazek]

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

11 ↗

(On Diff #77015)

Is there check-fixes-not? This seems to be required here, because even if the fixit won't happen here, the test will pass.

[• SilverGeri]

move checker to readability module
add missing description to header file

[• SilverGeri]

update tests with "CHECK-FIXES-NOT" parts

[hokein]

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

4

We don't rely on implementations of standard headers in lit test. You should fake the function/class that you need in this test.

8

Does it work the case like:

int *p = nullptr;
if (p == nullptr) {
   p = new int[3];
   delete[] p;
}

?

[aaron.ballman]

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

8

Similarly, it should not mishandle a case like:

void f(int *p) {

if (p) {
  delete p;
} else {
  // Do something else
}

}

74

Please add a newline.

[• SilverGeri]

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

8

it warns only if the compund statement contains only one statement (which is the delete). We want to warn because it is unnecessary to check the pointer validity if you want to just call delete. In other cases, we can't be sure about the actual behaviour.

[aaron.ballman]

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

8

In my example, the compound statement does contain only one statement, the delete, but diagnosing is likely a false positive due to the else clause. In that case, the pointer validity controls more than just the delete because it also controls whether to execute the else clause. Removing the if clause shouldn't be a mechanical change in the presence of an else clause, so the fixit is definitely inappropriate. I think that diagnosing is still pretty reasonable, however.

Another test case, which I think may already be handled appropriately but should still be explicitly tested:

if (p) {
  // This comment should not disable the check or the fixit.
  // Nor should this comment.
  delete p;
}

I think this check should still be able to remove the if clause, but we should make sure that the comments don't disable the check, and that the fixit doesn't destroy the comments.

[• SilverGeri]

only warn, not fix when the 'if' statement has 'else' clause
keeping comments

[hokein]

It looks good to me, but you'd better wait for the approval from @aaron.ballman.

clang-tidy/readability/DeleteNullPointerCheck.cpp

56

I would use an early return if (IfWithDelete->getElse()) return here.

[alexfh]

clang-tidy/readability/DeleteNullPointerCheck.cpp

53

Rename D to Diag, please.

56

Definitely use early exit.

57

This variable is unused.

73–77

Please clang-format the file.

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

21

Please truncate repeated static parts of the check patterns that exceed 80 characters (e.g. remove the deleting null pointer has no effect [readability-delete-null-pointer] part from all but the first CHECK line).

54

Please add CHECK-FIXES lines. Now there's no easy way to see from the test whether any fixes are applied here.

[• SilverGeri]

remove unused string
using early exit in condition
shorten check-message lines
add check-fisex to 'else' part

[Prazek]

clang-tidy/readability/DeleteNullPointerCheck.cpp

34

I think allOf matcher is redundant here because ifStmt takes variadic number of arguments and matches only if all of them matches.

[• SilverGeri]

removing redundant allOf from ifStmt

[aaron.ballman]

LGTM, with one nit. You should wait for @alexfh to sign off before committing though, since he requested changes.

clang-tidy/readability/DeleteNullPointerCheck.cpp

54

Elide the braces.

[• SilverGeri]

remove brackets

[xazax.hun]

Small ping, is this ready to be committed?

[aaron.ballman]

In D21298#632154, @xazax.hun wrote:

Small ping, is this ready to be committed?

If @alexfh doesn't sign off by tomorrow, I think it's fine to commit. We can deal with any last minute comments post-commit.

[alexfh]

clang-tidy/readability/DeleteNullPointerCheck.cpp

29

Since binaryOperator (and most if not all other node matchers) is VariadicDynCastAllOfMatcher, allOf is redundant here (similar to Piotr's comment below).

Same for compoundStmt below.

docs/clang-tidy/checks/readability-delete-null-pointer.rst

6

Enclose if in double backquotes instead of single quotes.

7

Either null pointer or nullptr (enclosed in double backquotes).

10

Insert an empty line above and check that Sphinx can build this.

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

7

This check (also combined with the ones below) is useless. It will work for the unchanged file as well: it will skip the if (p) line and find the comment, the next CHECK-FIXES will find the delete p; line and the CHECK-FIXES-NOT will find no lines matching if (p) between them.

I'd use something like this:

// comment1
if (p) { // comment 2
  delete p;
} // comment 3
// CHECK-FIXES: {{^  }}// comment1
// CHECK-FIXES-NEXT: {{^  }} // comment2
// CHECK-FIXES-NEXT: {{^  }}  delete p;
// CHECK-FIXES-NEXT: {{^  }} // comment3

Same for patterns below.

[alexfh]

In D21298#632235, @aaron.ballman wrote:

In D21298#632154, @xazax.hun wrote:

Small ping, is this ready to be committed?

If @alexfh doesn't sign off by tomorrow, I think it's fine to commit. We can deal with any last minute comments post-commit.

Yup, that's totally fine, especially when there was a thorough pre-commit code review.

[alexfh]

In D21298#632265, @alexfh wrote:

In D21298#632235, @aaron.ballman wrote:

In D21298#632154, @xazax.hun wrote:

Small ping, is this ready to be committed?

If @alexfh doesn't sign off by tomorrow, I think it's fine to commit. We can deal with any last minute comments post-commit.

Yup, that's totally fine, especially when there was a thorough pre-commit code review.

Clarification: it's totally fine to submit without waiting for me longer than a grace period of a day or so, once all comments are addressed and other reviewers have LGTM'd the patch.

However, here specifically I have a few more comments ;)

[• SilverGeri]

remove redundant allOf statements;
refactor test's comment checking part

[alexfh]

I've noticed a few more minor issues. Otherwise looks good.

Thank you for the new check!

clang-tidy/readability/DeleteNullPointerCheck.cpp

28–39

This can be simplified. Something like this (modulo formatting):

const auto PointerExpr = ignoringImpCasts(declRefExpr(to(decl().bind("deletedPointer"))));
const auto PointerCondition = castExpr(hasCastKind(CK_PointerToBoolean), hasSourceExpression(PointerExpr));
const auto BinaryPointerCheckCondition =
    binaryOperator(hasEitherOperand(castExpr(hasCastKind(CK_NullToPointer))),
                   hasEitherOperand(PointerExpr));

(and remove the second hasCondition).

54

The check can suggest a fix in this case as well, but it's a bit more involved. Please add a FIXME.

docs/clang-tidy/checks/readability-delete-null-pointer.rst

7

Sorry for not being clear enough: "null pointer" is not an inline code snippet, it shouldn't be enclosed in double backquotes or anything else. The "(enclosed in double backquotes)" part was meant to apply to nullptr only (since it's a keyword and should be highlighted as a code snippet).

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

9

The that should not be deleted part is superfluous, IMO. You could even leave just // #1, // #2, etc.

[• SilverGeri]

reduce number hasCondition to 1;
add FIXME comment;
shorten check comments in test

[• SilverGeri]

docs/clang-tidy/checks/readability-delete-null-pointer.rst

7

To be honest, I don't even understand why I did what I did... :D

[alexfh]

One more late comment (I should really add a check-list for new checks): this check lacks tests with macros and templates with multiple instantiations.

Incorrect handling of templates will likely not manifest in the current state of the check, it's brittle, since it relies on the error deduplication performed by clang-tidy and it can break easily (e.g. if message text will depend on the instantiation or if something changes in the way clang-tidy deduplicates messages). However, attempts to apply fixes to code resulting from macro expansions is unlikely to result in compilable code.

clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ContainerSizeEmptyCheck.cpp
+  DeleteNullPointerCheck.cpp
   DeletedDefaultCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolCastCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   MisplacedArrayIndexCheck.cpp
   NamedParameterCheck.cpp

clang-tidy/readability/DeleteNullPointerCheck.h

+//===--- DeleteNullPointerCheck.h - clang-tidy-------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_DELETE_NULL_POINTER_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.
+///
+/// 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-tidy/readability/DeleteNullPointerCheck.cpp

+//===--- DeleteNullPointerCheck.cpp - clang-tidy---------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "DeleteNullPointerCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void DeleteNullPointerCheck::registerMatchers(MatchFinder *Finder) {
+  const auto DeleteExpr =
+      cxxDeleteExpr(has(castExpr(has(declRefExpr(
+                        to(decl(equalsBoundNode("deletedPointer"))))))))
+          .bind("deleteExpr");
+
+  const auto PointerCondition = castExpr(hasCastKind(CK_PointerToBoolean));
+  const auto BinaryPointerCheckCondition = binaryOperator(
+      allOf(hasEitherOperand(castExpr(hasCastKind(CK_NullToPointer))),
+            hasEitherOperand(ignoringImpCasts(declRefExpr()))));
+
+  Finder->addMatcher(
+      ifStmt(
+          hasCondition(anyOf(PointerCondition, BinaryPointerCheckCondition)),
+          hasCondition(anyOf(
+              ignoringImpCasts(declRefExpr(to(decl().bind("deletedPointer")))),
+              binaryOperator(hasEitherOperand(ignoringImpCasts(
+                  declRefExpr(to(decl().bind("deletedPointer")))))))),
+          hasThen(anyOf(DeleteExpr, compoundStmt(allOf(has(DeleteExpr),
+                                                       statementCountIs(1)))
+                                        .bind("compound"))))
+          .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");
+
+  auto Diag = diag(
+      IfWithDelete->getLocStart(),
+      "'if' statement is unnecessary; deleting null pointer has no effect");
+  if (IfWithDelete->getElse())
+    return;
+
+  Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
+      IfWithDelete->getLocStart(),
+      Lexer::getLocForEndOfToken(IfWithDelete->getCond()->getLocEnd(), 0,
+                                 *Result.SourceManager,
+                                 Result.Context->getLangOpts())));
+  if (Compound) {
+    Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
+        Compound->getLBracLoc(),
+        Lexer::getLocForEndOfToken(Compound->getLBracLoc(), 0,
+                                   *Result.SourceManager,
+                                   Result.Context->getLangOpts())));
+    Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
+        Compound->getRBracLoc(),
+        Lexer::getLocForEndOfToken(Compound->getRBracLoc(), 0,
+                                   *Result.SourceManager,
+                                   Result.Context->getLangOpts())));
+  }
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tidy/readability/ReadabilityTidyModule.cpp

 //===--- ReadabilityTidyModule.cpp - clang-tidy ---------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidConstParamsInDecls.h"
 #include "BracesAroundStatementsCheck.h"
 #include "ContainerSizeEmptyCheck.h"
+#include "DeleteNullPointerCheck.h"
 #include "DeletedDefaultCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"

 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidConstParamsInDecls>(
         "readability-avoid-const-params-in-decls");
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "readability-braces-around-statements");
     CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
         "readability-container-size-empty");
+    CheckFactories.registerCheck<DeleteNullPointerCheck>(
+        "readability-delete-null-pointer");
     CheckFactories.registerCheck<DeletedDefaultCheck>(
         "readability-deleted-default");
     CheckFactories.registerCheck<ElseAfterReturnCheck>(
         "readability-else-after-return");
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");
     CheckFactories.registerCheck<IdentifierNamingCheck>(
         "readability-identifier-naming");

docs/clang-tidy/checks/list.rst

.. toctree::
    performance-for-range-copy
    performance-implicit-cast-in-loop
    performance-inefficient-string-concatenation
    performance-unnecessary-copy-initialization
    performance-unnecessary-value-param
    readability-avoid-const-params-in-decls
    readability-braces-around-statements
    readability-container-size-empty
+   readability-delete-null-pointer
    readability-deleted-default
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name
    readability-misplaced-array-index
    readability-named-parameter

docs/clang-tidy/checks/readability-delete-null-pointer.rst

+.. title:: clang-tidy - readability-delete-null-pointer
+
+readability-delete-null-pointer
+===============================
+
+Checks the 'if' statements where a pointer's existence is checked and then deletes the pointer.
+The check is unnecessary as deleting a nullpointer has no effect.
+
+.. code:: c++
+  int *p;
+  if (p)
+    delete p;

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

+// RUN: %check_clang_tidy %s readability-delete-null-pointer %t
+
+#define NULL 0
+
+void f() {
+  int *p = 0;
+  // CHECK-FIXES: // comment that should not be deleted
+  if (p) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'if' statement is unnecessary; deleting null pointer has no effect [readability-delete-null-pointer]
+    // comment that should not be deleted
+    delete p;
+  }
+  // CHECK-FIXES-NOT: if (p) {
+  // CHECK-FIXES: delete p;
+
+
+  int *p2 = new int[3];
+  // CHECK-FIXES: // another comment to keep
+  if (p2)
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'if' statement is unnecessary;
+    // another comment to keep
+    delete[] p2;
+  // CHECK-FIXES-NOT: if (p2)
+  // CHECK-FIXES: delete[] p2;
+
+  int *p3 = 0;
+  if (NULL != p3) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'if' statement is unnecessary;
+    delete p3;
+  }
+  // CHECK-FIXES-NOT: if (NULL != p3) {
+  // CHECK-FIXES: delete p3;
+
+  int *p4 = nullptr;
+  if (p4 != nullptr) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'if' statement is unnecessary;
+    delete p4;
+  }
+  // CHECK-FIXES-NOT: if (p4 != nullptr) {
+  // CHECK-FIXES: delete p4;
+
+  char *c;
+  if (c != 0) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'if' statement is unnecessary;
+    delete c;
+  }
+  // CHECK-FIXES-NOT: if (c != 0) {
+  // CHECK-FIXES: delete c;
+
+  char *c2;
+  if (c2) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'if' statement is unnecessary;
+    // CHECK-FIXES: } else {
+    // CHECK-FIXES: c2 = c;
+    delete c2;
+  } else {
+    c2 = c;
+  }
+}
+
+void g() {
+  int *p5, *p6;
+  if (p5)
+    delete p6;
+
+  if (p5 && p6)
+    delete p5;
+
+  if (p6) {
+    int x = 5;
+    delete p6;
+  }
+}