[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 12:14 PM, Alexander Kornienko <alexfh at google.com> wrote:
> On Thu, Sep 10, 2015 at 5:22 PM, Aaron Ballman <aaron.ballman at gmail.com>
> wrote:
>>
>> > ================
>> > 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.
>
>
> This side effect of the diag() method is one of the core things in the
> clang-tidy API for checks. The same pattern is used with other Diag/diag
> methods in clang that produce various DiagnosticBuilders, and so far I
> didn't see problems with it being misleading. So it shouldn't need a comment
> at each usage, imho. May be a comment at the method definition needs to
> cover this aspect as well, if it doesn't.

Yeah, I think what threw me off was the local variable declaration
more than the diag function call. In hindsight, this behavior makes
sense. We'll chalk it up to an education issue on my part, with no
changes needed.

Thanks!

~Aaron