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

[Ted Kremenek]

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