[cfe-commits] [PATCH] Improved __declspec support

[Aaron Ballman]

On Sun, Jun 17, 2012 at 6:07 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> Mostly just nitpicking:
>
>> Index: include/clang/Basic/DiagnosticParseKinds.td
>> ===================================================================
>> --- include/clang/Basic/DiagnosticParseKinds.td       (revision 158272)
>> +++ include/clang/Basic/DiagnosticParseKinds.td       (working copy)
>> @@ -481,6 +481,9 @@
>>    "introducing an attribute">;
>>  def err_alignas_pack_exp_unsupported : Error<
>>    "pack expansions in alignment specifiers are not supported yet">;
>> +def err_unknown_ms_declspec : Error<"unrecognized __declspec attribute %0">;
>
> This should probably be a Warning, and InGroup<UnknownAttributes>. It
> would also seems sensible to mirror the wording of
> warn_unknown_attribute_ignored: "unknown __declspec attribute %0
> ignored".

The reason I made it an error was because it's an error in VS as well.
 Since it pertains to __declspec, which is already MS-specific, I
figured the behavior should match.  Given that reasoning, do you still
think we should prefer a warning over an error?

>> Index: include/clang/Sema/AttributeList.h
>> ===================================================================
>> --- include/clang/Sema/AttributeList.h        (revision 158272)
>> +++ include/clang/Sema/AttributeList.h        (working copy)
>> @@ -80,6 +80,9 @@
>>    /// availability attribute.
>>    unsigned IsAvailability : 1;
>>
>> +  /// True if this attribute is actually a type attribute
>> +  unsigned IsTypeAttribute : 1;
>
> Sean Hunt has a patch for this class which will replace the
> DeclspecAttribute and CXX0XAttribute flags with an enum for the
> syntactic form used for the attribute. It would make sense to add this
> as a value in the enum, instead of adding another flag (so you're
> going to need to wait a short while for Sean's patch to land), since
> these attributes don't use the syntactic form of __declspec
> attributes. The comment (and possibly the name) could also be improved
> to indicate that these are Microsoft type attribute keywords (and give
> a couple of examples in the comment).

Fair enough.

> A general comment about this function: the MS documentation suggests
> that multiple attributes may be specified in the same __declspec, for
> instance "__declspec(dllexport naked)". It appears that the old code
> supported this and the new code doesn't. Is that intentional?

Great catch (it was unintentional) -- looks like I need another
testcase for this, too.

>> Index: lib/Sema/SemaDeclAttr.cpp
>> ===================================================================
>> --- lib/Sema/SemaDeclAttr.cpp (revision 158272)
>> +++ lib/Sema/SemaDeclAttr.cpp (working copy)
>> @@ -4162,8 +4164,11 @@
>>    if (Attr.isInvalid())
>>      return;
>>
>> -  if (Attr.isDeclspecAttribute() && !isKnownDeclSpecAttr(Attr))
>> -    // FIXME: Try to deal with other __declspec attributes!
>> +  // Type attributes are still treated as declaration attributes by
>> +  // ParseMicrosoftTypeAttributes and ParseBorlandTypeAttributes.  We don't
>> +  // want to process them, however, because we will simply warn about ignoring
>> +  // them.  So instead, we will bail out early.
>> +  if (Attr.isTypeAttribute())
>>      return;
>
> Can you check Attr.isUsedAsTypeAttr() instead, in the code path which
> produces the warning? This would presumably allow the checks for
> AT_address_space, AT_opencl_image_access etc in
> ProcessInheritableDeclAttr to be removed too.

Yes, I bet I can -- I'll look into it for the next patch.

Thanks again!

~Aaron