[PATCH] Disallowing just GNU-style attributes in certain positions

[Richard Smith]

On Mon, Jul 21, 2014 at 2:03 PM, Aaron Ballman <aaron at aaronballman.com>
wrote:

> I've cleaned the patch up and implemented your suggestions. Just to
> clarify something:
>
> > Index: lib/Parse/ParseDecl.cpp
> > ===================================================================
> > --- lib/Parse/ParseDecl.cpp (revision 213263)
> > +++ lib/Parse/ParseDecl.cpp (working copy)
> > @@ -5535,7 +5541,7 @@
> >    // If there is a type-qualifier-list, read it now.
> >    // Type qualifiers in an array subscript are a C99 feature.
> >    DeclSpec DS(AttrFactory);
> > -  ParseTypeQualifierListOpt(DS, false /*no attributes*/);
> > +  ParseTypeQualifierListOpt(DS, NoAttributesAllowed);
>
> The old code used to allow C++11-style attributes, but from what I can
> tell, those should have been prohibited here as well, so I switched to
> NoAttributesAllowed. Is my understanding correct? Is there a sensible
> testcase I should add for this?
>

I think that we need something like this to reach that code:

void f(int a[static [[]] 5]);

... where we produce an error on the 'static', but don't reject the
attribute list.


> Thanks! Assuming my understanding is correct, I intend to commit after
> 3.5 branches.
>
> ~Aaron
>
> On Thu, Jul 17, 2014 at 6:40 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > On Thu, Jul 17, 2014 at 11:36 AM, Aaron Ballman <aaron at aaronballman.com>
> > wrote:
> >>
> >> GNU-style type attributes are not allowed in new expressions, such as:
> >>
> >> auto *P = new int * __attribute__((attr));
> >>
> >> We silently accept them in clang, but have a FIXME in the code to
> >> address the issue. This patch is an initial stab at addressing the
> >> issue, and I am looking for feedback on whether my approach is
> >> reasonable or not. (It disables support for GNU attributes, but not
> >> other vendor attributes which should remain supported.)
> >>
> >> Note, the attached test case does not currently pass. It only checks
> >> for one error diagnostic, but the actual diagnostics for that code
> >> are:
> >>
> >> ..\llvm\tools\clang\test\SemaCXX\attr-gnu.cpp:4:22: error: an
> >> attribute list cannot appear here
> >>   auto P = new int * __attribute__((vector_size(8))); // expected-error
> >> ...
> >>                      ^
> >> ..\llvm\tools\clang\test\SemaCXX\attr-gnu.cpp:4:21: error: expected
> >> ';' at end of declaration
> >>   auto P = new int * __attribute__((vector_size(8))); // expected-error
> >> ...
> >>                     ^
> >>                     ;
> >> ..\llvm\tools\clang\test\SemaCXX\attr-gnu.cpp:4:22: warning:
> >> declaration does not declare anything [-Wmissing-declarations]
> >>   auto P = new int * __attribute__((vector_size(8))); // expected-error
> >> ...
> >>                      ^~~~~~~~~~~~~
> >> 1 warning and 2 errors generated.
> >>
> >> The first diagnostic is obviously correct, but are the other two
> >> reasonable diagnostics as well?
> >
> >
> > I would prefer for them to be suppressed: if you're in the
> > GNUAttributesProhibited case and you see a GNU attribute, I think you
> should
> > both parse it and diagnose it.
> >
> >> If the approach is reasonable, I'll clean the patch up, add more test
> >> cases, etc. I mostly wanted to check my direction first. Thanks!
> >
> >
> > The direction looks good to me. I think your AttrRequirements enum could
> be
> > made clearer (separate XAllowed and XProhibited flags seem a bit
> confusing
> > at first -- perhaps XParsed and XParsedAndRejected or something along
> those
> > lines might be clearer?)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140721/ae95f164/attachment.html>