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.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
