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

[Richard Smith via cfe-commits]

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

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

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/02071c86/attachment.html>