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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

The check is really awesome! Thank you for coming up with all the patterns that frequently happen to result from coding errors!

Please update release notes.

A few inline comments as well.


================
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:22
@@ +21,3 @@
+
+AST_MATCHER(QualType, isAnyChar) {
+  return Node->isCharType() || Node->isWideCharType() || Node->isChar16Type() ||
----------------
Seems like you could use the `isAnyCharacter` matcher.

================
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:27
@@ +26,3 @@
+
+AST_MATCHER(BinaryOperator, isRelationnalOperator) {
+  return Node.isRelationalOp();
----------------
nit: extra 'n' in "relational"

================
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:27
@@ +26,3 @@
+
+AST_MATCHER(BinaryOperator, isRelationnalOperator) {
+  return Node.isRelationalOp();
----------------
alexfh wrote:
> nit: extra 'n' in "relational"
I'd put this to ASTMatchers.h together with a test and docs.

================
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:40
@@ +39,3 @@
+  ASTContext::DynTypedNodeList Parents = Finder->getASTContext().getParents(*E);
+  if (Parents.size() != 1 || !(E = Parents[0].get<Expr>()))
+    return false;
----------------
The `!(E = ...)` construct is a bit hard to read (needs time to figure out the intention). I'd prefer a simpler alternative.

================
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:196
@@ +195,3 @@
+      expr(SizeOfExpr, unless(SizeOfZero),
+           hasSizeOfAncestor(SizeOfExpr.bind("sizeof-sizeof-expr"))),
+      this);
----------------
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.

================
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:236
@@ +235,3 @@
+      diag(E->getLocStart(), "suspicious usage of 'sizeof(...)/sizeof(...)';"
+                             " sizes are incompatible");
+    } else if (ElementSize > CharUnits::Zero() &&
----------------
I'm not sure I would understand what "incompatible" means here without reading the code of the check. Please expand the message. Same in the messages below.

================
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:258
@@ +257,3 @@
+                 Result.Nodes.getNodeAs<Expr>("sizeof-multiply-sizeof")) {
+    diag(E->getLocStart(), "suspicious multiplation of two 'sizeof'");
+  }
----------------
s/multiplation/multiplication/


http://reviews.llvm.org/D19014