<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Aug 26, 2015 at 11:27 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron.ballman@gmail.com" target="_blank">aaron.ballman@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">aaron.ballman created this revision.<br>
aaron.ballman added reviewers: rsmith, hans.<br>
aaron.ballman added a subscriber: cfe-commits.<br>
<br>
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:<br>
<br>
struct __attribute__((lockable)) __declspec(dllexport) S { }; // ok<br>
struct __declspec(dllexport) __attribute__((lockable)) T { }; // error about declaring anonymous structs and a warning about declarations not declaring anything<br>
<br>
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.<br>
<br>
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.<br>
<br>
However, this patch is not complete, as the test cases for function attributes currently fail for the C++11-style attributes.<br>
<br>
[[noreturn]] __attribute__((cdecl)) __declspec(dllexport) void f();<br>
<br>
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.<br>
<br>
__attribute__((cdecl)) [[noreturn]] __declspec(dllexport) void f();<br>
<br>
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.<br></blockquote><div><br></div><div>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.</div><div><br></div><div>I don't think that these cases are fundamentally different from this perspective:</div><div><br></div><div>[[noreturn]] inline void f();</div><div>[[noreturn]] __attribute__((gnu_inline)) void f();</div><div> </div><div><div>inline [[noreturn]] void f(); // ill-formed</div><div>__attribute__((gnu_inline)) [[noreturn]] void f(); // ill-formed?</div></div><div><br></div><div>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.</div><div><br></div><div>(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.)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
<br>
Before taking the process further, I was wondering about two things:<br>
<br>
1) Is the direction with MaybeParseAttributes() an acceptable approach?<br>
2) If it is, is it reasonable to commit that, and work on the ParseDeclarationSpecifiers() portion in a follow-up commit?<br>
<br>
Thanks!<br>
<br>
~Aaron<br>
<br>
<a href="http://reviews.llvm.org/D12375" rel="noreferrer" target="_blank">http://reviews.llvm.org/D12375</a><br>
<br>
Files:<br>
  include/clang/Parse/Parser.h<br>
  lib/Parse/ParseDecl.cpp<br>
  lib/Parse/ParseDeclCXX.cpp<br>
  lib/Parse/ParseExprCXX.cpp<br>
  lib/Parse/Parser.cpp<br>
  test/Parser/attr-order.cpp<br>
<br>
</blockquote></div><br></div></div>