[PATCH] Only strip underscores for GNU attributes

[Richard Smith]

On Wed, Dec 11, 2013 at 1:13 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 05/12/2013 20:16, Aaron Ballman wrote:
>
>> Currently, attribute names that have surrounding double underscores
>> will have them stripped silently when determining the attribute kind.
>> However, this doesn't make sense for all forms of attributes. For
>> instance: __declspec(__dllexport__) should not be a valid attribute
>> name, nor should [[__carries_dependency__]]
>>
>> This patch checks the attribute form and only strips the underscores
>> for GNU attributes. However, it does also handle C++11-style GNU
>> attribute as well, because there are existing test cases checking for
>> that. Do we want to have the same behavior for C++11-style clang
>> attributes?
>>
>> ~Aaron
>>
>
> Hi Aaron,
>
> Seems fine to do as you did and limit this to spellings of GNU attributes
> only as doing it for C++11 attributes was clearly wrong. We can then
> consider limiting it further for C++11 GNU spellings if there's a need (but
> my understanding is that there isn't.)
>

+1.

GCC accepts [[gnu::__attr__]], so I think we shouldn't limit this further.

(FWIW, GCC also accepts [[__noreturn__]]. I'm not sure whether that's a
bug, but they don't accept [[__gnu__::whatever], so I guess it probably is.)


> Small style nits below, LG otherwise..
>
>
>  Index: lib/Sema/AttributeList.cpp
>> ===================================================================
>> --- lib/Sema/AttributeList.cpp    (revision 196524)
>> +++ lib/Sema/AttributeList.cpp    (working copy)
>> @@ -121,14 +121,17 @@
>>                                             Syntax SyntaxUsed) {
>>    StringRef AttrName = Name->getName();
>>
>> -  // Normalize the attribute name, __foo__ becomes foo.
>> -  if (AttrName.size() >= 4 && AttrName.startswith("__") &&
>> +  SmallString<64> Buf;
>>
>
> Call this something like FullName, or anything more descriptive than Buf
> :-)
>
>  +  if (ScopeName)
>> +    Buf += ScopeName->getName();
>> +
>> +  // Normalize the attribute name, __foo__ becomes foo. This is only
>> allowable
>> +  // for GNU attributes.
>> +  if ((SyntaxUsed == AS_GNU || (SyntaxUsed == AS_CXX11 &&
>> Buf.equals("gnu"))) &&
>>
>
> Breaking the checks into an IsGNU bool would improve readability.
>
>
>  +      AttrName.size() >= 4 && AttrName.startswith("__") &&
>>        AttrName.endswith("__"))
>>      AttrName = AttrName.substr(2, AttrName.size() - 4);
>>
>
> AttrName.slice(2, AttrName.size() - 2) more readable? Your call.
>
> Alp.
>
>
>
>> -  SmallString<64> Buf;
>> -  if (ScopeName)
>> -    Buf += ScopeName->getName();
>>    // Ensure that in the case of C++11 attributes, we look for '::foo' if
>> it is
>>    // unscoped.
>>    if (ScopeName || SyntaxUsed == AS_CXX11)
>> Index: test/Sema/MicrosoftCompatibility.c
>> ===================================================================
>> --- test/Sema/MicrosoftCompatibility.c    (revision 196524)
>> +++ test/Sema/MicrosoftCompatibility.c    (working copy)
>> @@ -19,3 +19,5 @@
>>  struct __declspec(aligned) S2 {}; /* expected-warning {{unknown
>> __declspec attribute 'aligned' ignored}} */
>>
>>  struct __declspec(appdomain) S3 {}; /* expected-warning {{__declspec
>> attribute 'appdomain' is not supported}} */
>> +
>> +__declspec(__noreturn__) void f7(void); /* expected-warning {{unknown
>> __declspec attribute '__noreturn__' ignored}} */
>> Index: test/SemaCXX/attr-cxx0x.cpp
>> ===================================================================
>> --- test/SemaCXX/attr-cxx0x.cpp    (revision 196524)
>> +++ test/SemaCXX/attr-cxx0x.cpp    (working copy)
>> @@ -45,3 +45,6 @@
>>  static_assert(alignof(outer<int,char>::inner<double,short>) ==
>> alignof(int) * alignof(double), "template's alignment is wrong");
>>
>>  static_assert(alignof(int(int)) >= 1, "alignof(function) not
>> positive"); // expected-error{{invalid application of 'alignof' to a
>> function type}}
>> +
>> +[[__carries_dependency__]]  // expected-warning{{unknown attribute
>> '__carries_dependency__' ignored}}
>> +void func(void);
>>
>
>
>
>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131211/27432619/attachment.html>