[PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

[Aaron Ballman via cfe-commits]

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