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

[Eli Friedman]

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