On Thu, Sep 10, 2015 at 10:54 AM, Alexander Kornienko <ale...@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
>
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
