[PATCH] D21298: [Clang-tidy] delete null check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

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

Thank you for the new check!



================
Comment at: clang-tidy/readability/DeleteNullPointerCheck.cpp:27-38
+  const auto PointerCondition = castExpr(hasCastKind(CK_PointerToBoolean));
+  const auto BinaryPointerCheckCondition =
+      binaryOperator(hasEitherOperand(castExpr(hasCastKind(CK_NullToPointer))),
+                     hasEitherOperand(ignoringImpCasts(declRefExpr())));
+
+  Finder->addMatcher(
+      ifStmt(
----------------
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`).


================
Comment at: clang-tidy/readability/DeleteNullPointerCheck.cpp:53
+      "'if' statement is unnecessary; deleting null pointer has no effect");
+  if (IfWithDelete->getElse())
+    return;
----------------
The check can suggest a fix in this case as well, but it's a bit more involved. Please add a FIXME.


================
Comment at: docs/clang-tidy/checks/readability-delete-null-pointer.rst:7
+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.
+
----------------
alexfh wrote:
> Either `null pointer` or `nullptr` (enclosed in double backquotes).
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).


================
Comment at: test/clang-tidy/readability-delete-null-pointer.cpp:8
+
+  // comment that should not be deleted #1
+  if (p) { // comment that should not be deleted #2
----------------
The `that should not be deleted` part is superfluous, IMO. You could even leave just `// #1`, `// #2`, etc.


https://reviews.llvm.org/D21298