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

[Aaron Ballman via cfe-commits]

On Thu, Sep 10, 2015 at 10:54 AM, Alexander Kornienko <alexfh at google.com> wrote:
> alexfh marked 3 inline comments as done.
>
> ================
> Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:37
> @@ +36,3 @@
> +      expr(unless(isInTemplateInstantiation()),
> +           sizeOfExpr(
> +               has(expr(hasType(hasCanonicalType(hasDeclaration(recordDecl(
> ----------------
> Yes, I do. `sizeOfExpr().bind("...")` doesn't compile for some reason.
>
> ```
> tools/clang/tools/extra/clang-tidy/misc/SizeofContainerCheck.cpp:24:35: error: no member named 'bind' in 'clang::ast_matchers::internal::Matcher<clang::Stmt>'
>                   .bind("expr"))).bind("sizeof"),
>                   ~~~~~~~~~~~~~~~ ^
> ```

That's kind of strange, but okay!

> ================
> Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:39
> @@ +38,3 @@
> +               has(expr(hasType(hasCanonicalType(hasDeclaration(recordDecl(
> +                   matchesName("std::(basic_string|vector)|::string")))))))))
> +          .bind("sizeof"),
> ----------------
> I'd like to limit this to the STL containers first. So "any recordDecl named std::.* with a .size() method" might work.

Sounds reasonable to me.

> ================
> Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:49
> @@ +48,3 @@
> +  SourceLocation SizeOfLoc = SizeOf->getLocStart();
> +  auto Diag = diag(SizeOfLoc, "sizeof() doesn't return the size of the "
> +                              "container. Did you mean .size()?");
> ----------------
> 1. Done.
> 2. It's actually emitting the diagnostic. And I thought that the comment on line 52 below explains what happens in enough detail (`// Don't generate fixes for macros.`). If something is still unclear, can you explain what exactly?

It's not intuitive that creating the local variable actually emits the
diagnostic, so it seems like you would be able to hoist the early
return up above the local declaration when in fact that would remove
the diagnostic entirely. The comment about generating fixes suggests
an additional diagnostic, at least to me.

>
>
> http://reviews.llvm.org/D12759
>
>
>