<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jul 21, 2014 at 2:03 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I've cleaned the patch up and implemented your suggestions. Just to<br>
clarify something:<br>
<br>
> Index: lib/Parse/ParseDecl.cpp<br>
> ===================================================================<br>
> --- lib/Parse/ParseDecl.cpp (revision 213263)<br>
> +++ lib/Parse/ParseDecl.cpp (working copy)<br>
> @@ -5535,7 +5541,7 @@<br>
>    // If there is a type-qualifier-list, read it now.<br>
>    // Type qualifiers in an array subscript are a C99 feature.<br>
>    DeclSpec DS(AttrFactory);<br>
> -  ParseTypeQualifierListOpt(DS, false /*no attributes*/);<br>
> +  ParseTypeQualifierListOpt(DS, NoAttributesAllowed);<br>
<br>
The old code used to allow C++11-style attributes, but from what I can<br>
tell, those should have been prohibited here as well, so I switched to<br>
NoAttributesAllowed. Is my understanding correct? Is there a sensible<br>
testcase I should add for this?<br></blockquote><div><br></div><div>I think that we need something like this to reach that code:</div><div><br></div><div>void f(int a[static [[]] 5]);</div><div><br></div><div>... where we produce an error on the 'static', but don't reject the attribute list.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks! Assuming my understanding is correct, I intend to commit after<br>
3.5 branches.<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Thu, Jul 17, 2014 at 6:40 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> On Thu, Jul 17, 2014 at 11:36 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> GNU-style type attributes are not allowed in new expressions, such as:<br>
>><br>
>> auto *P = new int * __attribute__((attr));<br>
>><br>
>> We silently accept them in clang, but have a FIXME in the code to<br>
>> address the issue. This patch is an initial stab at addressing the<br>
>> issue, and I am looking for feedback on whether my approach is<br>
>> reasonable or not. (It disables support for GNU attributes, but not<br>
>> other vendor attributes which should remain supported.)<br>
>><br>
>> Note, the attached test case does not currently pass. It only checks<br>
>> for one error diagnostic, but the actual diagnostics for that code<br>
>> are:<br>
>><br>
>> ..\llvm\tools\clang\test\SemaCXX\attr-gnu.cpp:4:22: error: an<br>
>> attribute list cannot appear here<br>
>>   auto P = new int * __attribute__((vector_size(8))); // expected-error<br>
>> ...<br>
>>                      ^<br>
>> ..\llvm\tools\clang\test\SemaCXX\attr-gnu.cpp:4:21: error: expected<br>
>> ';' at end of declaration<br>
>>   auto P = new int * __attribute__((vector_size(8))); // expected-error<br>
>> ...<br>
>>                     ^<br>
>>                     ;<br>
>> ..\llvm\tools\clang\test\SemaCXX\attr-gnu.cpp:4:22: warning:<br>
>> declaration does not declare anything [-Wmissing-declarations]<br>
>>   auto P = new int * __attribute__((vector_size(8))); // expected-error<br>
>> ...<br>
>>                      ^~~~~~~~~~~~~<br>
>> 1 warning and 2 errors generated.<br>
>><br>
>> The first diagnostic is obviously correct, but are the other two<br>
>> reasonable diagnostics as well?<br>
><br>
><br>
> I would prefer for them to be suppressed: if you're in the<br>
> GNUAttributesProhibited case and you see a GNU attribute, I think you should<br>
> both parse it and diagnose it.<br>
><br>
>> If the approach is reasonable, I'll clean the patch up, add more test<br>
>> cases, etc. I mostly wanted to check my direction first. Thanks!<br>
><br>
><br>
> The direction looks good to me. I think your AttrRequirements enum could be<br>
> made clearer (separate XAllowed and XProhibited flags seem a bit confusing<br>
> at first -- perhaps XParsed and XParsedAndRejected or something along those<br>
> lines might be clearer?)<br>
</div></div></blockquote></div><br></div></div>