aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a comment.
Great idea for a checker! Some comments below.
================
Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:36
@@ +35,3 @@
+ Finder->addMatcher(
+ expr(sizeOfExpr(
+ has(expr(hasType(hasCanonicalType(hasDeclaration(recordDecl(
----------------
Do you need the expr() matcher?
================
Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:38
@@ +37,3 @@
+ has(expr(hasType(hasCanonicalType(hasDeclaration(recordDecl(
+ matchesName("std::(basic_string|vector)|::string")))))))))
+ .bind("sizeof"),
----------------
I think that this should be checking for more than just basic_string and
vector. Why not other containers? For instance, anything that exposes a member
function with a size() function seems like it would be reasonable.
================
Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:48
@@ +47,3 @@
+ SourceLocation SizeOfLoc = SizeOf->getLocStart();
+ auto Diag = diag(SizeOfLoc, "sizeof() doesn't return the size of the "
+ "container. Did you mean .size()?");
----------------
I think the period should be replaced with a semicolon (it's not a sentence, so
the period is also wrong). e.g. (with some wording tweaks),
"sizeof() does not return the size of the container; did you mean to call
size()?"
Also, is this actually emitting the diagnostic? If so, a comment would be good
explaining why the early return for macros is located where it is, so no one
does a drive-by "fix."
http://reviews.llvm.org/D12759
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
