[PATCH] Only strip underscores for GNU attributes

[Aaron Ballman]

Revised patch attached with Alp's suggests applied.

As for accepting [[__noreturn__]], I would say that's not something we
want to emulate.

There's only one attribute which has a GNU, C++11 gnu and C++11 clang
spelling: warn_unused_result. Given that, I think this patch is
reasonable to not strip the underscores from C++11 clang spellings,
but if we find that breaks a lot of code in practice, it's easy enough
to resolve.

~Aaron

On Wed, Dec 11, 2013 at 4:20 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 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 --------------
A non-text attachment was scrubbed...
Name: AttrUnderscores.patch
Type: application/octet-stream
Size: 2647 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131211/6f7f698f/attachment.obj>