On Thu, Sep 10, 2015 at 12:14 PM, Alexander Kornienko <ale...@google.com> wrote: > On Thu, Sep 10, 2015 at 5:22 PM, Aaron Ballman <aaron.ball...@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
