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

[Sam Panzer]

Ping.

On Wed, Jun 6, 2012 at 6:35 PM, Sam Panzer <panzer at google.com> wrote:

> Thanks again for the input.
>
> Of the two main issues,
> (1) the POD_Type enum
> I used the enum to distinguish between three cases: one requires a warning
> for C++98 compatibility, one requires the standard
> can't-pass-non-pod-to-vararg warning, and the third does nothing. Since the
> code that checks for non-POD arguments to vararg functions lives in two
> places because of the way format-string checking is implemented, I thought
> it made sense to separate the pod-kind-determining logic from the
> warning-emitting logic.
>
> Does someone have a suggestion for implementing this? I can think of two
> other approaches, and neither seems as clean: levelOfTriviality could
> either take an extra argument that tells it not to emit warnings, or it
> could take a series of callbacks for the three cases.
>
> (2) It is possible for SemaChecking to reuse the logic from
> hasFormatStringArgument(), since both of them really compute the (expected)
> location of the format string. I refactored a bit to share the relevant
> logic, though there isn't too much in common. Is this an improvement?
>
> On Wed, Jun 6, 2012 at 5:20 PM, Chandler Carruth <chandlerc at google.com>wrote:
>
>> Some minor comments, and I'll let others start chiming in here to make
>> sure there is general agreement about the approach:
>>
>>
>> +def warn_non_pod_vararg_with_format_string : Warning<
>> +  "cannot pass non-POD object of type %0 to variadic function; "
>> +  "expected type was %1">,
>>
>> I'd like to mention that the expectation stems from a format string here:
>>
>> '... variadic function; the format string expects it to have type %1'
>>
>> or some such...
>>
>> +  // Used for determining in which context a type is considered POD
>> +  enum PODType {
>> +    POD,
>> +    CXX11_POD,
>> +    ObjC_Lifetime_POD,
>> +    NON_POD
>> +  };
>>
>> The LLVM coding conventions on enums are much more precise than other
>> things. I would vaguely expect it to look like:
>>
>> enum PODKind {
>>   PK_NonPOD,
>>   PK_POD,
>>   PK_CXX11POD,
>>   PK_ObjCLifetimePOD
>> };
>>
>> +
>> +  // Determines which PODType fits an expression.
>> +  PODType levelOfTriviality(const Expr *E);
>> +
>>
>> Actually, looking at how this function is implemented, I think you're
>> going about it the wrong way. This isn't a generalizable concept, it is a
>> very specific and precise query: isValidTypeForVarargArgument or some such.
>> It should return true or false, no enum required.
>>
>
>> +                                              bool HasformatString =
>> false);
>>
>> s/HasformatString/HasFormatString/
>>
>> +// Given a variadic function, decides whether it looks like we have a
>> +// printf-style format string and arguments)
>> +bool Sema::hasFormatStringArgument(FunctionDecl *FDecl,
>> +                                   Expr **Args, unsigned NumArgs) {
>>
>> I don't see you using this function from anywhere but new code -- is
>> there a reason that SemaChecking can't use this logic?
>>
>> +    // Decide if we're probably inspecting a printf-like function
>> +
>> +    bool HasFormatString = hasFormatStringArgument(FDecl, Args, NumArgs);
>>
>> I think the comment is now extraneous.
>>
>>
>> On Wed, Jun 6, 2012 at 4:56 PM, Sam Panzer <panzer at google.com> wrote:
>>
>>> Parens removed, though operator-> is unavailable here. For future
>>> reference, is Clang averse to extra variable declarations in favor of extra
>>> method calls?
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120613/501bcb08/attachment.html>