[PATCH] D12375: [PATCH] Relax parse ordering rules for attributes

[Richard Smith via cfe-commits]

On Wed, Aug 26, 2015 at 1:55 PM, Aaron Ballman <aaron.ballman at gmail.com>
wrote:

> On Wed, Aug 26, 2015 at 4:30 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > On Wed, Aug 26, 2015 at 11:27 AM, Aaron Ballman <aaron.ballman at gmail.com
> >
> > wrote:
> >>
> >> aaron.ballman created this revision.
> >> aaron.ballman added reviewers: rsmith, hans.
> >> aaron.ballman added a subscriber: cfe-commits.
> >>
> >> PR24559 brings up a long-standing issue in Clang where attribute order
> >> with differing syntax impacts whether a parse will succeed or fail with
> poor
> >> diagnostics. For instance:
> >>
> >> struct __attribute__((lockable)) __declspec(dllexport) S { }; // ok
> >> struct __declspec(dllexport) __attribute__((lockable)) T { }; // error
> >> about declaring anonymous structs and a warning about declarations not
> >> declaring anything
> >>
> >> When you consider attributes as extraneous semantic information to
> attach
> >> to entities, ordering of the attributes really should not matter
> between the
> >> various attribute syntaxes we support -- they all serve the same
> purpose.
> >>
> >> This patch begins to address the issue by providing a
> >> MaybeParseAttributes() function accepting a mask of what attribute
> syntaxes
> >> are allowed and parses the various syntaxes in a loop.
> >>
> >> However, this patch is not complete, as the test cases for function
> >> attributes currently fail for the C++11-style attributes.
> >>
> >> [[noreturn]] __attribute__((cdecl)) __declspec(dllexport) void f();
> >>
> >> In this case, the [[noreturn]] is parsed as part of the top-level
> >> declaration, while the GNU and declspec attributes are parsed as part
> of the
> >> declaration specifier, and everything is accepted.
> >>
> >> __attribute__((cdecl)) [[noreturn]] __declspec(dllexport) void f();
> >>
> >> In this case, nothing is parsed as part of the top-level declaration,
> and
> >> everything is parsed as part of the declaration specifier. However, the
> C++
> >> attribute is prohibited from appearing on a declaration specifier and
> so is
> >> diagnosed.
> >
> > That seems like pedantically correct behavior to me. C++ attributes at
> the
> > start of the declaration appertain to the declared entities; C++
> attributes
> > after a decl-specifier, declarator, or id-expression appertain to that
> > decl-specifier, declarator, or entity. Conversely, a GNU attribute or MS
> > attribute (in this position) is a decl-specifier.
>
> I agree with that assessment, thank you for reminding me that C++
> attributes can apply to decl-specifiers. I still find it unsatisfying
> from a usability perspective at the same time. While the GNU or MS
> attribute in that position is a decl-specifier, it's also an
> attribute. Do we want users thinking about what position an attribute
> needs to be in to not be a decl-specifier?


In an ideal world, we'd get people to stop using the non-C++11 attribute
syntaxes entirely -- then the problem would not arise :)

Then again, any reordering
> we allow now would be really painful to deal with for anyone
> (including the standards body) who wants an attribute to appertain to
> the decl-specifier.
>
> > I don't think that these cases are fundamentally different from this
> > perspective:
> >
> > [[noreturn]] inline void f();
> > [[noreturn]] __attribute__((gnu_inline)) void f();
> >
> > inline [[noreturn]] void f(); // ill-formed
> > __attribute__((gnu_inline)) [[noreturn]] void f(); // ill-formed?
> >
> > Rejecting the last of these cases may not seem especially user-friendly,
> but
> > we should do everything we can to avoid C++11 attributes becoming the
> morass
> > of syntactic soup that __attribute__ has become.
>
> That's definitely true and compelling, and I agree that we need to
> tread more lightly than I was. Perhaps a different solution isn't to
> allow arbitrary orders, but to instead provide a fixit suggesting the
> "correct" (whatever that means with all these vendor extensions being
> used at once) ordering. However, such a mechanism would still require
> us to be able to parse funky orders in order to slide things around to
> where they should be (and would make it really hard to have a C++
> attribute apply to a decl-specifier), so that idea may be problematic
> and time consuming too.


It'd be good to at least diagnose these attribute order issues better, even
if we can't easily provide a fix-it hint or recover elegantly.

> (Also, I'd note that allowing reordering of C++11 and GNU attributes is
> not
> > GCC-compatible; our current permitted ordering is intended to be GCC
> > compatible. If we go ahead with your patch, you should at least add a
> > -Wgcc-compat warning for it.)
>
> If I understand correctly, it seems like __attribute__ and __declspec
> should always be interchangeable, while [[]] should never be (and I
> have no ideas on Microsoft [] attributes as there's no grammar, but I
> could research this)? If that's the case, I can provide a modified
> patch and some good test cases with comments explaining what's going
> on.
>

That sounds like a good approach to me. It also matches MinGW behavior as I
recall it (one of __declspec and __attribute__ is #defined to the other, so
they can appear in either order).


> Thanks!
>
> ~Aaron
>
> >
> >> I think ParseDeclarationSpecifiers() needs to accept a
> >> ParsedAttributesWithRange object representing the top-level declaration
> >> attributes, and the parser needs to attach the C++ attributes to it up
> until
> >> we the first non-attribute token is found. But I'm not 100% certain
> that's
> >> the right approach.
> >>
> >> Before taking the process further, I was wondering about two things:
> >>
> >> 1) Is the direction with MaybeParseAttributes() an acceptable approach?
> >> 2) If it is, is it reasonable to commit that, and work on the
> >> ParseDeclarationSpecifiers() portion in a follow-up commit?
> >>
> >> Thanks!
> >>
> >> ~Aaron
> >>
> >> http://reviews.llvm.org/D12375
> >>
> >> Files:
> >>   include/clang/Parse/Parser.h
> >>   lib/Parse/ParseDecl.cpp
> >>   lib/Parse/ParseDeclCXX.cpp
> >>   lib/Parse/ParseExprCXX.cpp
> >>   lib/Parse/Parser.cpp
> >>   test/Parser/attr-order.cpp
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150826/f0b786f8/attachment.html>