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 <[email protected]> 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 <[email protected]>wrote: > >> On Mon, Jun 4, 2012 at 2:09 PM, Sam Panzer <[email protected]> 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 >>> <[email protected]>wrote: >>> >>>> On Mon, Jun 4, 2012 at 1:37 PM, Sam Panzer <[email protected]> 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. >>>> >>> >>> >> >
printf-update.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
