[PATCH] Only strip underscores for GNU attributes

[Aaron Ballman]

Thanks! I've committed in r197082 and will keep an eye out for issues
this may raise.

~Aaron

On Wed, Dec 11, 2013 at 5:25 PM, Alp Toker <alp at nuanti.com> wrote:
>
> 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
>