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

[Sam Panzer]

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/20120606/644c53bb/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: printf-update-2.patch
Type: application/octet-stream
Size: 22718 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120606/644c53bb/attachment.obj>