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
