[PATCH] Only strip underscores for GNU attributes

[Alp Toker]

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

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