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

[Etienne Bergeron via cfe-commits]

etienneb added a comment.

Some comments after a quick look to the code.


================
Comment at: clang-tidy/misc/DeleteNullCheck.cpp:22
@@ +21,3 @@
+void DeleteNullCheck::registerMatchers(MatchFinder *Finder) {
+  const auto HasDeleteExpr =
+      cxxDeleteExpr(hasDescendant(declRefExpr().bind("pointerToDelete")))
----------------
HasDeleteExpr -> DeleteExpr

================
Comment at: clang-tidy/misc/DeleteNullCheck.cpp:23
@@ +22,3 @@
+  const auto HasDeleteExpr =
+      cxxDeleteExpr(hasDescendant(declRefExpr().bind("pointerToDelete")))
+          .bind("deleteExpr");
----------------
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.

================
Comment at: clang-tidy/misc/DeleteNullCheck.cpp:47
@@ +46,3 @@
+
+  if ((CastExpr->getCastKind() == CastKind::CK_PointerToBoolean) &&
+      (PointerToDelete->getDecl() == Cond->getDecl()) &&
----------------
This check could be moved to the matcher part above/
see ASTMatcher: 	hasCastKind

================
Comment at: clang-tidy/misc/DeleteNullCheck.cpp:48
@@ +47,3 @@
+  if ((CastExpr->getCastKind() == CastKind::CK_PointerToBoolean) &&
+      (PointerToDelete->getDecl() == Cond->getDecl()) &&
+      (!Compound || Compound->size() == 1)) {
----------------
see: equalsBoundNode

================
Comment at: clang-tidy/misc/DeleteNullCheck.cpp:49
@@ +48,3 @@
+      (PointerToDelete->getDecl() == Cond->getDecl()) &&
+      (!Compound || Compound->size() == 1)) {
+    auto D = diag(
----------------
This check should be moved to the matcher part too.
see: 	statementCountIs


Repository:
  rL LLVM

http://reviews.llvm.org/D21298