[PATCH] Only strip underscores for GNU attributes

[Alp Toker]

On 11/12/2013 21:43, Aaron Ballman wrote:
> Revised patch attached with Alp's suggests applied.
>
> As for accepting [[__noreturn__]], I would say that's not something we
> want to emulate.

Aaron, thanks for the fixes. There's general agreement it's not a kludge 
we want if we can avoid it.

>
> 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.

LGTM.

(Obviously let's keep an eye out over the next few days though to see if 
anything was relying on the old behaviour.)

Alp.


> ~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
>>>

-- 
http://www.nuanti.com
the browser experts