This is split off from D90188.
Details
Diff Detail
Event Timeline
Can you mention this change in the release notes? Also, should we document it explicitly in the Language Extensions documentation, or do you think this is too tiny of a feature to warrant that?
clang/include/clang/Parse/Parser.h | ||
---|---|---|
2648 | In general, things named with CXX11Attributes really mean double square bracket attributes more than C++, so they also impact [[]] behavior in C. I think the function name here is fine (it's just as bad as the other names) but the comment should probably also include C2x attributes. | |
clang/lib/Parse/ParseDecl.cpp | ||
1606 | And the implementation side of things should probably be checking isC2xAttribute() as well. | |
1607 | Can you also pass in << PA.getRange()? (This ensures that the whole attribute is underlined for the diagnostic instead of just the first token of the attribute, which may be just the vendor namespace.) |
clang/test/Parser/cxx0x-attributes.cpp | ||
---|---|---|
134 | Btw, a -pedantic test for the parser behavior would be useful so that we can be sure the new diagnostic is firing as expected. |
Thanks for the patch!
No further comments regarding the patch, except the items Aaron already mentioned.
Do [[deprecated]] and [[maybe_unused]] now work for using-declarators? If so, a test for that would be useful.
clang/lib/Parse/ParseDeclCXX.cpp | ||
---|---|---|
714 | Should we suggest moving the attributes after the identifier in this case (even though that will leave the program ill-formed), as we do for alias-declarations? | |
737 | Should the prefix attributes go before the suffix attributes? (I could imagine there might be attributes for which order matters.) What do we do in the simple-declaration case? |
I think the answer is yes and no, respectively (at least in terms of whether the attribute will apply to the subject) -- but [[clang::annotate("whatever")]] is also a good test attribute as would be any attribute whose Subjects includes Named (for NamedDecls).
Sure, I added release notes and language extension docs. I also added a __has_extension for this. That might all be a little overkill, I'm happy to pull out those changes if so.
[[deprecated]] gets applied, but it doesn't have any effect, since we don't attach an attribute to the UsingShadowDecl, and regardless, we already look past the UsingShadowDecl before we reach DiagnoseUseOfDecl. I added a -Wignored-attributes diagnostic to call this out.
clang/lib/Parse/ParseDeclCXX.cpp | ||
---|---|---|
714 | Sure, I added this in the new patch. | |
737 | Surprisingly, this does apply attributes to the beginning of the list: void addAll(iterator B, iterator E) { AttrList.insert(AttrList.begin(), B.I, E.I); } This is consistent with simple-declarations. | |
clang/test/Parser/cxx0x-attributes.cpp | ||
134 | Sure, I added that in a new test file. |
LGTM aside from a few minor NFC changes and a documentation request. Thank you for this!
clang/docs/LanguageExtensions.rst | ||
---|---|---|
623 | Wrong number of = here -- should extend them to the end of the heading. | |
630 | Can you also add using foo::bar [[clang::using_if_exists]]; to show that we support the syntax in two locations? | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
2419 | isa<UsingDecl, UnresolvedUsingTypenameDecl, UnresolvedUsingValueDecl>(D) | |
6952 | Same suggestion here. |
Gentle ping! Can we merge this? I'd love to move forward with https://reviews.llvm.org/D90257.
Address @aaron.ballman's review comments. Also, fix a bug where we failed to properly parse an inherited constructor declaration with a trailing attribute.
LGTM with a minor improvement for attribute ordering.
clang/lib/Parse/ParseDeclCXX.cpp | ||
---|---|---|
732–733 | You should replace these two lines with a call to: MaybeParseAttributes(PAKM_GNU | PAKM_CXX11, Attrs); so that order of GNU vs CXX syntax doesn't matter (they can be interleaved). |
This is breaking check-clang everywhere, eg https://lab.llvm.org/buildbot/#/builders/109/builds/15757
Please run tests before landing, or failing that watch the bots after committing.
Reverted in f63adf5b67f7a25b15f81d3a1a207aba4f226dc4 for now.
This is also failing on several buildbots: https://lab.llvm.org/buildbot/#/builders/139/builds/4812
Since it's been failing for a few hours, I'd like to revert this in the near future to unclog our internal CI and the buildbots.
EDIT: @thakis beat me to it...
Sorry about that, I thought the pre-commit CI here on Phabricator had passed those tests successfully. The only failures I was seeing were related to clang-format. I'm not sure what happened. Anyway, thanks for reverting, I'll take a look at this.
Wrong number of = here -- should extend them to the end of the heading.