[cfe-commits] Patch: PR9751: -Wformat warnings will point to the callsite and give a note if the format string is elsewhere.

[Richard Trieu]

Committed at r143168.
On Wed, Oct 26, 2011 at 9:04 PM, Ted Kremenek <kremenek at apple.com> 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 <rtrieu at google.com>
> wrote:
> >>
> >>
> >> On Thu, Oct 13, 2011 at 5:18 PM, Richard Trieu <rtrieu at google.com>
> wrote:
> >>>
> >>> On Thu, Oct 13, 2011 at 3:25 PM, Richard Trieu <rtrieu at google.com>
> wrote:
> >>>>
> >>>> On Thu, Oct 13, 2011 at 2:53 PM, Eli Friedman <eli.friedman at gmail.com
> >
> >>>> wrote:
> >>>>>
> >>>>> On Thu, Oct 13, 2011 at 2:38 PM, Richard Trieu <rtrieu at google.com>
> >>>>> 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
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111027/d8bd21bd/attachment.html>