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

[Etienne Bergeron via cfe-commits]

etienneb added inline comments.

================
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:27
@@ +26,3 @@
+
+AST_MATCHER(BinaryOperator, isRelationnalOperator) {
+  return Node.isRelationalOp();
----------------
alexfh wrote:
> alexfh wrote:
> > nit: extra 'n' in "relational"
> I'd put this to ASTMatchers.h together with a test and docs.
There are a few matchers used in different checkers that worth lifting.
It seems there is a place to lift some of them in clang-tidy/utils, and some of them should be lifted to astmatcher directly.
I wonder what should be the rules to lift them.

================
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:196
@@ +195,3 @@
+      expr(SizeOfExpr, unless(SizeOfZero),
+           hasSizeOfAncestor(SizeOfExpr.bind("sizeof-sizeof-expr"))),
+      this);
----------------
alexfh wrote:
> It looks like you've created a tool for what can be done in a simpler way. It should be possible to express this matcher without going through the parent map: `sizeOfExpr(hasDescendant(sizeOfExpr(unless(SizeOfZero))))` or something like that.
This is 'almost' working!
We cannot pass through all expression.
As an example, callExpr or arrayExpr

```
sizeof( A[index(sizeof(int))] )
```

In this example, the checker won't warn.

So, the hasDescendant (or has ancestor) in this case is only allow to follow a subset of Expressions.

The name may be wrong, or unclear. If you have better proposition...

================
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:197
@@ +196,3 @@
+      this);
+}
+
----------------
alexfh wrote:
> What about this comment?
unsubmited. (not sure to get when arc is pushing them)


http://reviews.llvm.org/D19014