[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

mboehme added a comment. Fix in review at https://reviews.llvm.org/D128499 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126061/new/ https://reviews.llvm.org/D126061 ___ cfe-commits mailing list cfe-comm

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

mboehme added a comment. In D126061#3605276 , @miyuki wrote: > Hi Martin, > > I think this patch caused a regression in diagnostics. Clang used to warn > about the following code: [snip] > And now it doesn't. Could you please have a look into it? Than

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

aaron.ballman added a comment. In D126061#3605276 , @miyuki wrote: > Hi Martin, > > I think this patch caused a regression in diagnostics. Clang used to warn > about the following code: > > struct X { > [[deprecated]] struct Y {}; > }; > > > >

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

miyuki added a comment. Hi Martin, I think this patch caused a regression. Clang used to warn about the following code: struct X { [[deprecated]] struct Y {}; }; $ clang -std=c++17 -Wall test.cpp :2:7: warning: attribute 'deprecated' is ignored, place it after "struct" to appl

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

mboehme added a comment. https://reviews.llvm.org/D128097 is now submitted. Unfortunately, it didn't fix the compile-time regression entirely (see detailed comments there). I'll do some more investigation to see where the rest of the slowdown is coming from. Repository: rG LLVM Github Monor

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

mboehme added a comment. Fix for compile time regression submitted for review at https://reviews.llvm.org/D128097 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126061/new/ https://reviews.llvm.org/D126061 _

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

mboehme added a comment. In D126061#3585681 , @nikic wrote: > FYI this change had a measurable effect on compile-time > (http://llvm-compile-time-tracker.com/compare.php?from=7acc88be0312c721bc082ed9934e381d297f4707&to=8c7b64b5ae2a09027c38db969a04fc9ddd0

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

aaron.ballman added inline comments. Comment at: clang/lib/Parse/Parser.cpp:1164 DS.getParsedSpecifiers() == DeclSpec::PQ_StorageClassSpecifier) { -Decl *TheDecl = ParseLinkage(DS, DeclaratorContext::File); +Decl *TheDecl = ParseLinkage(DS, DeclaratorContext::File,

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

mboehme added a comment. In D126061#3590896 , @dyung wrote: > @mboehme, one of our internal tests started to fail to compile after this > change due to the compiler no longer accepting what I think should be a valid > attribute declaration. I have filed

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

dyung added a comment. @mboehme, one of our internal tests started to fail to compile after this change due to the compiler no longer accepting what I think should be a valid attribute declaration. I have filed issue #56077 for the problem, can you take a look? Repository: rG LLVM Github Mo

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

mboehme added a comment. In D126061#3585681 , @nikic wrote: > FYI this change had a measurable effect on compile-time > (http://llvm-compile-time-tracker.com/compare.php?from=7acc88be0312c721bc082ed9934e381d297f4707&to=8c7b64b5ae2a09027c38db969a04fc9ddd0

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

nikic added a comment. FYI this change had a measurable effect on compile-time (http://llvm-compile-time-tracker.com/compare.php?from=7acc88be0312c721bc082ed9934e381d297f4707&to=8c7b64b5ae2a09027c38db969a04fc9ddd0cd6bb&stat=instructions), about 0.5% regression for `O0` builds. Not sure if that's

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

mboehme marked 2 inline comments as done. mboehme added a comment. Thank you @aaron.ballman and @rsmith for the careful and productive reviews! Comment at: clang/lib/Parse/ParseStmt.cpp:229 default: { +bool HaveAttrs = !(CXX11Attrs.empty() && GNUAttrs.empty()); +auto

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! Comment at: clang/include/clang/Basic/AttributeCommonInfo.h:153 // FIXME: Use a better mechanism to determine this. +// We use this in `isCXX11Attribute` below, so it _should_ only return +/

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

mboehme marked 11 inline comments as done. mboehme added a comment. In D126061#3581350 , @aaron.ballman wrote: > Only thing left for me are the nits I pointed out in the last review, > otherwise I think this is all set. Thanks for the quick turnaround!

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

aaron.ballman accepted this revision. aaron.ballman added a comment. Only thing left for me are the nits I pointed out in the last review, otherwise I think this is all set. Giving my LG because I don't think we need another round of review for those nits unless you want it C

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. This is a bold direction but I like it a lot. Over to @aaron.ballman for final approval. Comment at: clang/lib/Sema/ParsedAttr.cpp:228 + switch (getParsedKind()) { + case

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

aaron.ballman added a comment. In D126061#3577054 , @mboehme wrote: > @aaron.ballman Any further comments from you on this change? Mostly just nits from me at this point, plus some answers to questions I had missed previously. > There is some breakage

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

mboehme added a comment. @aaron.ballman Any further comments from you on this change? There is some breakage in the pre-merge checks, but it looks as if it's pre-existing breakage. See the comment-only change here that looks as if it exhibits similar breakage: https://reviews.llvm.org/D127494

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

mboehme marked an inline comment as done. mboehme added inline comments. Comment at: clang/lib/Sema/ParsedAttr.cpp:228 + switch (getParsedKind()) { + case AT_AddressSpace: + case AT_OpenCLPrivateAddressSpace: rsmith wrote: > I don't think this one really slide

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

rsmith added inline comments. Comment at: clang/lib/Parse/ParseStmt.cpp:242-246 +Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, CXX11Attrs, +GNUAttrs, &GNUAttributeLoc); } else { -Decl = ParseDeclaration(Declarato

[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

mboehme marked 8 inline comments as done. mboehme added a comment. I notice that https://reviews.llvm.org/D111548, which this change is based on, has some failing flang tests in the CI. I'm pretty sure these are unrelated and stem from the base revision that I happen to be based off. Before reba