[cfe-commits] [patch] Added a suggestion when a std::string is passed to printf()

[Sam Panzer]

And here is another update that replaces the default non-POD error with a
smarter (printf-specific) one, rather than just tacking on an extra warning.

-Sam

On Mon, Jun 4, 2012 at 6:17 PM, Sam Panzer <panzer at google.com> wrote:

> It turns out that intercepting the non-POD error is quite a bit more
> complicated than getting the basic functionality of this patch to work, so
> I decided to work on that as a separate patch.
>
> Here is the newer patch. The important features include:
>  - When a non-POD argument is passed to a printf-like function, it is
> checked for a c_str() member function that takes no parameters and returns
> a type that makes printf happy, the extra note is emitted with a FixItHint.
>  - This works for standard and wide-char strings (and anything else that
> people have thought up), since it lets analyze_printf::ArgTypeResult figure
> out the type matching.
>  - I don't check for conversion operators, since we (usually) get a char*
> out of c_str().
>  - Sema gained a function to find all C++ members of a given kind, a
> generalization of what I used previously.
>
> What I'm missing for Chandler's request of combining the error and warning
> is:
>  - Code that detects that it is looking at a printf-like varargs function.
> Something similar appears in lib/Sema/SemaChecking.cpp: it looks like
> iterating over specific_attr_iterator<FormatAttr> as happens
> in Sema::CheckFunctionCall is the best option. Is this correct?
>
> Thanks again!
>
>
> On Mon, Jun 4, 2012 at 2:10 PM, Chandler Carruth <chandlerc at google.com>wrote:
>
>> On Mon, Jun 4, 2012 at 2:09 PM, Sam Panzer <panzer at google.com> wrote:
>>
>>> Now I'm wondering how much of this behavior should be "string"-specific.
>>> If we're paying attention to conversion operators from non-POD types, why
>>> not check for all possible conversion operators? I think this is a better
>>> solution than checking to see if there's a conversion operator from the
>>> return type of a c_str() function.
>>>
>>
>> I think conversion operators aren't really as interesting and at least
>> should be handled as a separate follow-on patch. I also think they're
>> completely uninteresting when considering the result of 'c_str()'. All of
>> the interesting cases actually return the character pointer we're looking
>> for.
>>
>>
>>>
>>> Not depending on std::string was the plan, and in fact how I implemented
>>> it :)
>>>
>>> On Mon, Jun 4, 2012 at 1:44 PM, Chandler Carruth <chandlerc at google.com>wrote:
>>>
>>>> On Mon, Jun 4, 2012 at 1:37 PM, Sam Panzer <panzer at google.com> wrote:
>>>>
>>>>> Here is one other question: With this patch, incorrectly passing an
>>>>> std::string to printf generates an error (can't pass non-POD member), a
>>>>> warning (printf format doesn't match), and a note (the c_str() suggestion).
>>>>> Is this the behavior we want?
>>>>
>>>>
>>>> I don't think so... Ideally:
>>>>
>>>> 1) If we are about to issue a an error (non-POD object passed through
>>>> printf), check whether the non-POD object has a 'c_str()' method that
>>>> returns a type which matches the format specifier. If so, use a specific
>>>> diagnostic message for the error, attach a fixit-hint suggesting
>>>> '.c_str()', and continue parsing as-if the user had done that.
>>>>
>>>> 2) If there is no error (passing a std::string* perhaps), then in the
>>>> warning message, give a more precise message than 'cannot convert
>>>> std::string* to const char*' or whatever by checking if there is a c_str()
>>>> method that matches the type of the printf.
>>>>
>>>>
>>>> One thing I would encourage you to do: don't base this on
>>>> 'std::string'. I actually think the error/warning should fire for any class
>>>> type with a c_str method that returns a viable type. That way non-standard
>>>> string libraries will get the same benefit if they conform the the same
>>>> conceptual interface as the standard string library.
>>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120606/ee13e7c5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: printf-update.patch
Type: application/octet-stream
Size: 14473 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120606/ee13e7c5/attachment.obj>