<div class="gmail_quote">Committed at r143168.</div><div class="gmail_quote">On Wed, Oct 26, 2011 at 9:04 PM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">Richard,<br>
<br>
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.<br>
<br>
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.<br>
<br>
Overall looks great. Nice improvement.<br>
<div class="HOEnZb"><div class="h5"><br>
On Oct 25, 2011, at 1:12 PM, Eli Friedman wrote:<br>
<br>
> On Tue, Oct 25, 2011 at 12:46 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
>><br>
>><br>
>> On Thu, Oct 13, 2011 at 5:18 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
>>><br>
>>> On Thu, Oct 13, 2011 at 3:25 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
>>>><br>
>>>> On Thu, Oct 13, 2011 at 2:53 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>><br>
>>>> wrote:<br>
>>>>><br>
>>>>> On Thu, Oct 13, 2011 at 2:38 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>><br>
>>>>> wrote:<br>
>>>>>> <a href="http://llvm.org/bugs/show_bug.cgi?id=9751" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=9751</a><br>
>>>>>> -Wformat warnings will point to the format string, but no message at<br>
>>>>>> the<br>
>>>>>> call site. This patch will move the warning to always point at the<br>
>>>>>> call<br>
>>>>>> site. If the format string is part of the function call, one warning<br>
>>>>>> will<br>
>>>>>> show. If the format string is defined elsewhere, a warning at the<br>
>>>>>> call site<br>
>>>>>> plus a note where the format string is defined.<br>
>>>>>> Also, if a note is present, the fix-it would be moved to the note so<br>
>>>>>> they<br>
>>>>>> won't be applied with -fixit. If the format string was used in<br>
>>>>>> multiple<br>
>>>>>> places, fixits may cause problems in other places.<br>
>>>>>> printf("%d %d", 1);<br>
>>>>>> warning: more '%' conversions than data arguments [-Wformat]<br>
>>>>>> printf("%d %d", 1);<br>
>>>>>> ~^<br>
>>>>>> const char kFormat[] = "%d %d";<br>
>>>>>> printf(kFormat, 1);<br>
>>>>>> test2.c:8:8: warning: more '%' conversions than data arguments<br>
>>>>>> [-Wformat]<br>
>>>>>> printf(kFormat, 1);<br>
>>>>>> ^~~~~~~<br>
>>>>>> test2.c:7:29: note: format string is defined here<br>
>>>>>> const char kFormat[] = "%d %d";<br>
>>>>>> ~^<br>
>>>>>> Patch attached an available at <a href="http://codereview.appspot.com/5277043/" target="_blank">http://codereview.appspot.com/5277043/</a><br>
>>>>><br>
>>>>> +namespace {<br>
>>>>> + class StringLiteralFinder<br>
>>>>> + : public ConstStmtVisitor<StringLiteralFinder> {<br>
>>>>> + Sema& S;<br>
>>>>> + const StringLiteral *StringExpr;<br>
>>>>> + bool StringFound;<br>
>>>>> + public:<br>
>>>>> [...]<br>
>>>>><br>
>>>>> Why is StringLiteralFinder necessary? The caller should know how it<br>
>>>>> found the string literal...<br>
>>>>><br>
>>>> That information isn't currently preserved through the diagnostics. The<br>
>>>> StringLiteralFinder is necessary to determine this after the fact. I can go<br>
>>>> look again and see if I can propagate that bit of info through the code.<br>
>>>><br>
>>><br>
>>> Turns out, it was easier to to pass around a bool with this info than<br>
>>> I originally thought. New patch attached.<br>
>><br>
>> Ping.<br>
><br>
> I was sort of hoping someone more familiar with this code would take a<br>
> look... but a quick review:<br>
><br>
> + FixItHint FixIt) {<br>
> + //if (StringLiteralFinder(S, StringExpr, ArgumentExpr).HasString())<br>
> + if (inFunctionCall)<br>
><br>
> Forgot to get rid of that line?<br>
><br>
> Other than that, I don't see any issues. Thanks for writing<br>
> high-quality testcases.<br>
><br>
> -Eli<br>
><br>
</div></div><div class="HOEnZb"><div class="h5">> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
</div></div></blockquote></div><br>