Committed at r143168. On Wed, Oct 26, 2011 at 9:04 PM, Ted Kremenek <[email protected]> wrote:
> Richard, > > Thanks for writing this. Overall this looks excellent. I agree with Eli's > assessment; the only thing I had fault with was the stray comment. > > A bit of QOI: can you add a comment above EmitFormatDiagnostic, describing > what it does, and *why* it is templated. It looks like a bit of magic to > someone not familiar with the code, and a little prose will make it for more > intelligible to someone glancing at the code. > > Overall looks great. Nice improvement. > > On Oct 25, 2011, at 1:12 PM, Eli Friedman wrote: > > > On Tue, Oct 25, 2011 at 12:46 PM, Richard Trieu <[email protected]> > wrote: > >> > >> > >> On Thu, Oct 13, 2011 at 5:18 PM, Richard Trieu <[email protected]> > wrote: > >>> > >>> On Thu, Oct 13, 2011 at 3:25 PM, Richard Trieu <[email protected]> > wrote: > >>>> > >>>> On Thu, Oct 13, 2011 at 2:53 PM, Eli Friedman <[email protected] > > > >>>> wrote: > >>>>> > >>>>> On Thu, Oct 13, 2011 at 2:38 PM, Richard Trieu <[email protected]> > >>>>> wrote: > >>>>>> http://llvm.org/bugs/show_bug.cgi?id=9751 > >>>>>> -Wformat warnings will point to the format string, but no message at > >>>>>> the > >>>>>> call site. This patch will move the warning to always point at the > >>>>>> call > >>>>>> site. If the format string is part of the function call, one > warning > >>>>>> will > >>>>>> show. If the format string is defined elsewhere, a warning at the > >>>>>> call site > >>>>>> plus a note where the format string is defined. > >>>>>> Also, if a note is present, the fix-it would be moved to the note so > >>>>>> they > >>>>>> won't be applied with -fixit. If the format string was used in > >>>>>> multiple > >>>>>> places, fixits may cause problems in other places. > >>>>>> printf("%d %d", 1); > >>>>>> warning: more '%' conversions than data arguments [-Wformat] > >>>>>> printf("%d %d", 1); > >>>>>> ~^ > >>>>>> const char kFormat[] = "%d %d"; > >>>>>> printf(kFormat, 1); > >>>>>> test2.c:8:8: warning: more '%' conversions than data arguments > >>>>>> [-Wformat] > >>>>>> printf(kFormat, 1); > >>>>>> ^~~~~~~ > >>>>>> test2.c:7:29: note: format string is defined here > >>>>>> const char kFormat[] = "%d %d"; > >>>>>> ~^ > >>>>>> Patch attached an available at > http://codereview.appspot.com/5277043/ > >>>>> > >>>>> +namespace { > >>>>> + class StringLiteralFinder > >>>>> + : public ConstStmtVisitor<StringLiteralFinder> { > >>>>> + Sema& S; > >>>>> + const StringLiteral *StringExpr; > >>>>> + bool StringFound; > >>>>> + public: > >>>>> [...] > >>>>> > >>>>> Why is StringLiteralFinder necessary? The caller should know how it > >>>>> found the string literal... > >>>>> > >>>> That information isn't currently preserved through the diagnostics. > The > >>>> StringLiteralFinder is necessary to determine this after the fact. I > can go > >>>> look again and see if I can propagate that bit of info through the > code. > >>>> > >>> > >>> Turns out, it was easier to to pass around a bool with this info than > >>> I originally thought. New patch attached. > >> > >> Ping. > > > > I was sort of hoping someone more familiar with this code would take a > > look... but a quick review: > > > > + FixItHint FixIt) { > > + //if (StringLiteralFinder(S, StringExpr, ArgumentExpr).HasString()) > > + if (inFunctionCall) > > > > Forgot to get rid of that line? > > > > Other than that, I don't see any issues. Thanks for writing > > high-quality testcases. > > > > -Eli > > > > _______________________________________________ > > cfe-commits mailing list > > [email protected] > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
