[PATCH] D19014: [clang-tidy] Add new checker for suspicious sizeof expressions

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:28
@@ +27,3 @@
+  return Node.getValue().getZExtValue() > N;
+}
+
----------------
There are no firm rules. It mostly depends on how generic/useful in other tools the matcher could be. This one seems pretty generic and it could be used in a couple of other clang-tidy checks at least (e.g. readability/ContainerSizeEmptyCheck.cpp), so it seems to be a good fit for ASTMatchers.h. This can also be done as a separate step, if you prefer.

================
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:197
@@ +196,3 @@
+      this);
+}
+
----------------
I see the reasoning, but in this case I'd still prefer if this custom matcher looked at the children instead of the parents, since it's a more effective approach in general (at least because it doesn't use the parent map).

We should probably add a variant of `hasDescendant` (and maybe `hasAncestor`) with a way to limit which nodes it can look through.

================
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:257
@@ +256,3 @@
+                 Result.Nodes.getNodeAs<Expr>("sizeof-multiply-sizeof")) {
+    diag(E->getLocStart(), "suspicious 'sizeof' by 'sizeof' expression");
+  }
----------------
"'sizeof' by 'sizeof' expression" is ambiguous, "'sizeof' by 'sizeof' multiplication" would be better.


http://reviews.llvm.org/D19014