<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Aug 26, 2015 at 1:55 PM, 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"><div class="HOEnZb"><div class="h5">On Wed, Aug 26, 2015 at 4:30 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> On Wed, Aug 26, 2015 at 11:27 AM, Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.com</a>><br>
> wrote:<br>
>><br>
>> 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<br>
>> with differing syntax impacts whether a parse will succeed or fail with poor<br>
>> diagnostics. For instance:<br>
>><br>
>> struct __attribute__((lockable)) __declspec(dllexport) S { }; // ok<br>
>> struct __declspec(dllexport) __attribute__((lockable)) T { }; // error<br>
>> about declaring anonymous structs and a warning about declarations not<br>
>> declaring anything<br>
>><br>
>> When you consider attributes as extraneous semantic information to attach<br>
>> to entities, ordering of the attributes really should not matter between the<br>
>> various attribute syntaxes we support -- they all serve the same purpose.<br>
>><br>
>> This patch begins to address the issue by providing a<br>
>> MaybeParseAttributes() function accepting a mask of what attribute syntaxes<br>
>> are allowed and parses the various syntaxes in a loop.<br>
>><br>
>> However, this patch is not complete, as the test cases for function<br>
>> 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<br>
>> declaration, while the GNU and declspec attributes are parsed as part of the<br>
>> 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<br>
>> everything is parsed as part of the declaration specifier. However, the C++<br>
>> attribute is prohibited from appearing on a declaration specifier and so is<br>
>> diagnosed.<br>
><br>
> That seems like pedantically correct behavior to me. C++ attributes at the<br>
> start of the declaration appertain to the declared entities; C++ attributes<br>
> after a decl-specifier, declarator, or id-expression appertain to that<br>
> decl-specifier, declarator, or entity. Conversely, a GNU attribute or MS<br>
> attribute (in this position) is a decl-specifier.<br>
<br>
</div></div>I agree with that assessment, thank you for reminding me that C++<br>
attributes can apply to decl-specifiers. I still find it unsatisfying<br>
from a usability perspective at the same time. While the GNU or MS<br>
attribute in that position is a decl-specifier, it's also an<br>
attribute. Do we want users thinking about what position an attribute<br>
needs to be in to not be a decl-specifier?</blockquote><div><br></div><div>In an ideal world, we'd get people to stop using the non-C++11 attribute syntaxes entirely -- then the problem would not arise :)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Then again, any reordering<br>
we allow now would be really painful to deal with for anyone<br>
(including the standards body) who wants an attribute to appertain to<br>
the decl-specifier.<br>
<span class=""><br>
> I don't think that these cases are fundamentally different from this<br>
> perspective:<br>
><br>
> [[noreturn]] inline void f();<br>
> [[noreturn]] __attribute__((gnu_inline)) void f();<br>
><br>
> inline [[noreturn]] void f(); // ill-formed<br>
> __attribute__((gnu_inline)) [[noreturn]] void f(); // ill-formed?<br>
><br>
> Rejecting the last of these cases may not seem especially user-friendly, but<br>
> we should do everything we can to avoid C++11 attributes becoming the morass<br>
> of syntactic soup that __attribute__ has become.<br>
<br>
</span>That's definitely true and compelling, and I agree that we need to<br>
tread more lightly than I was. Perhaps a different solution isn't to<br>
allow arbitrary orders, but to instead provide a fixit suggesting the<br>
"correct" (whatever that means with all these vendor extensions being<br>
used at once) ordering. However, such a mechanism would still require<br>
us to be able to parse funky orders in order to slide things around to<br>
where they should be (and would make it really hard to have a C++<br>
attribute apply to a decl-specifier), so that idea may be problematic<br>
and time consuming too.</blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> (Also, I'd note that allowing reordering of C++11 and GNU attributes is not<br>
> GCC-compatible; our current permitted ordering is intended to be GCC<br>
> compatible. If we go ahead with your patch, you should at least add a<br>
> -Wgcc-compat warning for it.)<br>
<br>
</span>If I understand correctly, it seems like __attribute__ and __declspec<br>
should always be interchangeable, while [[]] should never be (and I<br>
have no ideas on Microsoft [] attributes as there's no grammar, but I<br>
could research this)? If that's the case, I can provide a modified<br>
patch and some good test cases with comments explaining what's going<br>
on.<br></blockquote><div><br></div><div>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).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks!<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
>> I think ParseDeclarationSpecifiers() needs to accept a<br>
>> ParsedAttributesWithRange object representing the top-level declaration<br>
>> attributes, and the parser needs to attach the C++ attributes to it up until<br>
>> we the first non-attribute token is found. But I'm not 100% certain that's<br>
>> 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<br>
>> 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>
><br>
</div></div></blockquote></div><br></div></div>