Ping.
On Wed, Jun 6, 2012 at 6:35 PM, Sam Panzer <pan...@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 <chandl...@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 <pan...@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?
>>
>>
>>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
