With clang-cl, this parses fine: struct __attribute__((lockable)) __declspec(dllexport) S { }; while this does not: struct __declspec(dllexport) __attribute__((lockable)) T { }; /tmp/a.cc(3,1) : error: declaration of anonymous struct must be a definition struct __declspec(dllexport) __attribute__((lockable)) T { }; ^ /tmp/a.cc(3,1) : warning: declaration does not declare anything [-Wmissing-declarations] struct __declspec(dllexport) __attribute__((lockable)) T { }; ^ It would be great if we could allow both orderings, or at least provide a better error message.
(In reply to comment #0) > With clang-cl, this parses fine: > > struct __attribute__((lockable)) __declspec(dllexport) S { }; > > while this does not: > > struct __declspec(dllexport) __attribute__((lockable)) T { }; > > /tmp/a.cc(3,1) : error: declaration of anonymous struct must be a definition > struct __declspec(dllexport) __attribute__((lockable)) T { }; > ^ > /tmp/a.cc(3,1) : warning: declaration does not declare anything > [-Wmissing-declarations] > struct __declspec(dllexport) __attribute__((lockable)) T { }; > ^ > Are you running into this with a particular library, or is this an issue you happened upon? > It would be great if we could allow both orderings, or at least provide a > better error message. From a user perspective, I think it definitely makes sense to allow arbitrary orderings of attributes (where applicable).
(In reply to comment #1) > (In reply to comment #0) > > With clang-cl, this parses fine: > > > > struct __attribute__((lockable)) __declspec(dllexport) S { }; > > > > while this does not: > > > > struct __declspec(dllexport) __attribute__((lockable)) T { }; > > > > /tmp/a.cc(3,1) : error: declaration of anonymous struct must be a definition > > struct __declspec(dllexport) __attribute__((lockable)) T { }; > > ^ > > /tmp/a.cc(3,1) : warning: declaration does not declare anything > > [-Wmissing-declarations] > > struct __declspec(dllexport) __attribute__((lockable)) T { }; > > ^ > > > > Are you running into this with a particular library, or is this an issue you > happened upon? It just came up in Chromium. I'll try to work around by reordering for now.
(In reply to comment #2) > (In reply to comment #1) > > Are you running into this with a particular library, or is this an issue you > > happened upon? > > It just came up in Chromium. I'll try to work around by reordering for now. I think that's a good solution for the time being. At some point, it might be nice to add a ParseAttributes(mask)-like function that allows for eldritch horrors like: struct __attribute__((lockable)) [[foo]] __declspec(dllexport) __attribute__((bar)) S {}; The function would basically parse all attributes that meet the mask requirements, instead of requiring explicit calls to parse each attribute spelling. I'm not certain how feasible this idea is in practice just yet, though.
I have a patch in progress that fixes this particular issue, but there's a deeper underlying problem with attribute parsing. [[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. 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. I will explore further, and propose a patch to the lists to at least see if we all agree on the direction I've taken.
*** Bug 24925 has been marked as a duplicate of this bug. ***
If it's any easier to only handle the __declspec __attribute__ case at first, it might be worth doing that first. Those are a lot more common than C++11 attributes in practice. If it's easier to improve the diagnostic than accepting either ordering, that would also be useful. The current experience for getting this wrong is pretty bad: https://codereview.chromium.org/1363363002/ So if there was some way to make it less bad sooner, it might be worth doing that first and fixing it for real later :-)
(In reply to comment #6) > If it's any easier to only handle the __declspec __attribute__ case at > first, it might be worth doing that first. Those are a lot more common than > C++11 attributes in practice. > > If it's easier to improve the diagnostic than accepting either ordering, > that would also be useful. > > The current experience for getting this wrong is pretty bad: > https://codereview.chromium.org/1363363002/ So if there was some way to make > it less bad sooner, it might be worth doing that first and fixing it for > real later :-) I haven't had the chance to keep working on this patch (work has me on other priorities currently, so this is a spare time patch for me currently), but here's what I've gotten so far: http://reviews.llvm.org/D12375 It's reasonably far along, I just need to finish up the comments Richard had on it. You're welcome to steal the patch from me and put on the finishing touches if you have time. Otherwise, I am hoping to get to it sooner rather than later, but that may be a month away.
This popped up again: https://reviews.llvm.org/D94788
This should be resolved by 9f2c7effd7f386e95aff3358500bc30974d35b0d