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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang-tidy/readability/DeleteNullPointerCheck.cpp:29
+  const auto BinaryPointerCheckCondition = binaryOperator(
+      allOf(hasEitherOperand(castExpr(hasCastKind(CK_NullToPointer))),
+            hasEitherOperand(ignoringImpCasts(declRefExpr()))));
----------------
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.


================
Comment at: docs/clang-tidy/checks/readability-delete-null-pointer.rst:6
+
+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.
----------------
Enclose `if` in double backquotes instead of single quotes.


================
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.
+
----------------
Either `null pointer` or `nullptr` (enclosed in double backquotes).


================
Comment at: docs/clang-tidy/checks/readability-delete-null-pointer.rst:10
+.. code:: c++
+  int *p;
+  if (p)
----------------
Insert an empty line above and check that Sphinx can build this.


================
Comment at: test/clang-tidy/readability-delete-null-pointer.cpp:7
+  int *p = 0;
+  // CHECK-FIXES: // comment that should not be deleted
+  if (p) {
----------------
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.


https://reviews.llvm.org/D21298