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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
