aaron.ballman added a comment. Thank you for your work on this, I generally like this new approach and think we're heading in the right direction.
================ Comment at: clang/include/clang/Sema/DeclSpec.h:1853-1856 /// Attrs - Attributes. ParsedAttributes Attrs; + ParsedAttributes DeclarationAttrs; ---------------- We should probably rename `Attrs` to be less generic and add some comments to explain why there are two lists of parsed attributes. ================ Comment at: clang/include/clang/Sema/ParsedAttr.h:657-658 + /// "slides" to the decl-specifier-seq). + /// Attributes with GNU, __declspec or keyword syntax generally slide + /// to the decl-specifier-seq. C++11 attributes specified ahead of the + /// declaration always appertain to the declaration according to the standard, ---------------- rsmith wrote: > I don't think this sentence is correct: "Attributes with GNU, __declspec or > keyword syntax generally slide to the decl-specifier-seq." Instead, I think > those attribute syntaxes are never parsed as declaration attributes in the > first place, so there is no possibility of "sliding" anywhere -- they simply > always are decl-spec attributes. That's fair -- I tend to think "sliding" is also what happens (at least notionally) in a case like `__attribute__((address_space(1))) int *i;` because it's a type attribute rather than a declaration attribute, but that's also a declaration specifier, so it doesn't really slide anywhere. ================ Comment at: clang/lib/Parse/ParseDecl.cpp:2188 // Parse the next declarator. - D.clear(); + D.clearExceptDeclarationAttrs(); D.setCommaLoc(CommaLoc); ---------------- rsmith wrote: > I wonder if a name like `prepareForNextDeclarator` or `clearForComma` would > be better here -- something that indicates why we're clearing rather than > describing how. Personally, I like `prepareForNextDeclarator()` -- that's nice and clear (to me). ================ Comment at: clang/lib/Parse/ParseDecl.cpp:4323-4326 + // C2x draft 6.7.2.1/9 : "The optional attribute specifier sequence in a + // member declaration appertains to each of the members declared by the + // member declarator list; it shall not appear if the optional member + // declarator list is omitted." ---------------- Good catch! This also applies in C++: http://eel.is/c++draft/class#mem.general-14 I think you should add some test coverage for this, along the lines of: ``` struct S { [[clang::annotate("test")]] int; // The attribute should be diagnosed (as an error?) }; ``` ================ Comment at: clang/lib/Parse/ParseDecl.cpp:6978-6997 // Parse any C++11 attributes. - MaybeParseCXX11Attributes(DS.getAttributes()); + ParsedAttributes ArgDeclAttrs(AttrFactory); + MaybeParseCXX11Attributes(ArgDeclAttrs); - // Skip any Microsoft attributes before a param. - MaybeParseMicrosoftAttributes(DS.getAttributes()); - - SourceLocation DSStart = Tok.getLocation(); + ParsedAttributes ArgDeclSpecAttrs(AttrFactory); // If the caller parsed attributes for the first argument, add them now. ---------------- rsmith wrote: > Seems to be mostly pre-existing, but I don't think this is right. The > `FirstArgAttrs` are all decl-specifier attributes (they're parsed in > `ParseParenDeclarator`, where we look for GNU attributes and `__declspec` > attributes), so if we parsed any of those, we should not now parse any syntax > that is supposed to precede decl-specifier attributes. The current code > allows attributes to appear in the wrong order in the case where we need to > disambiguate a paren declarator: https://godbolt.org/z/bzK6n8obM (note that > the `g` case properly errors, but the `f` case that needs lookahead to > determine whether the `(` is introducing a function declarator incorrectly > accepts misplaced attributes). Good catch! ================ Comment at: clang/lib/Parse/ParseObjc.cpp:1233-1234 // Now actually move the attributes over. takeDeclAttributes(attrs, D.getMutableDeclSpec().getAttributes()); + takeDeclAttributes(attrs, D.getDeclarationAttributes()); takeDeclAttributes(attrs, D.getAttributes()); ---------------- rsmith wrote: > I think we should keep the attributes in appearance order. +1 FWIW, we run into some "fun" bugs with attribute orders and declaration merging where the orders are opposite and it causes problems as in https://godbolt.org/z/58jTM4sGM (note how the error and the note swap positions), so the order of attributes tends to be important (both for semantic behavior as well as diagnostic behavior). ================ Comment at: clang/lib/Parse/ParseStmt.cpp:198 + ParsedAttributes Attrs(AttrFactory); + ConcatenateAttributes(CXX11Attrs, GNUAttrs, Attrs); + ---------------- I *think* this is going to be okay because attributes at the start of a statement have a very specific ordering, but one thing I was slightly worried about is attribute orders getting mixed up if the caller used something like `ParseAttributes()` or `MaybeParseAttributes()` where you can interleave the syntaxes. If this turns out to be a real issue at some point, I suppose we could have `ConcatenateAttributes()` do insertions in sort order based on source locations, but I think it'd be best to avoid that unless we see a real issue. ================ Comment at: clang/lib/Parse/ParseStmt.cpp:242-246 + Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, CXX11Attrs, + GNUAttrs, &GNUAttributeLoc); } else { - Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, Attrs); + Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, CXX11Attrs, + GNUAttrs); ---------------- rsmith wrote: > I think this is the only place where we're passing decl-specifier-seq > attributes into `ParseDeclaration`. There are only two possible cases here: > > 1) We have a simple-declaration, and can `ParseSimpleDeclaration` directly. > 2) We have a static-assert, using, or namespace alias declaration, which > don't permit attributes at all. > > So I think we *could* simplify this so that decl-spec attributes are never > passed into `ParseDeclaration`: > > * If the next token is `kw_static_assert`, `kw_using`, or `kw_namespace`, > then prohibit attributes and call `ParseDeclaration`. > * Otherwise, call `ParseSimpleDeclaration` and pass in the attributes. > * Remove the `DeclSpecAttrs` list from `ParseDeclaration`. > > I'm not requesting a change here -- I'm not sure whether that's a net > improvement or not -- but it seems worth considering. I think this is a good avenue to explore -- passing in two different attribute lists means someone will at some point get it wrong by accident, so only having one attribute list reduces the chances for bugs later. I don't imagine static assertions or namespaces will get leading attributes. However... I think asm-declaration and using-directive are also a bit special -- they're allowed to have leading attributes: http://eel.is/c++draft/dcl.dcl#nt:asm-declaration and http://eel.is/c++draft/dcl.dcl#nt:using-directive Do we also have to handle opaque-enum-declaration here? http://eel.is/c++draft/dcl.dcl#nt:block-declaration ================ Comment at: clang/lib/Parse/ParseStmt.cpp:317-318 case tok::kw_asm: { - ProhibitAttributes(Attrs); + ProhibitAttributes(CXX11Attrs); + ProhibitAttributes(GNUAttrs); bool msAsm = false; ---------------- It seems like we might have a pre-existing bug here for [[]] attributes? http://eel.is/c++draft/dcl.dcl#nt:asm-declaration ================ Comment at: clang/lib/Parse/ParseStmt.cpp:329-330 case tok::kw___if_not_exists: - ProhibitAttributes(Attrs); + ProhibitAttributes(CXX11Attrs); + ProhibitAttributes(GNUAttrs); ParseMicrosoftIfExistsStatement(Stmts); ---------------- Seeing this pattern over and over again... I wonder if we want a variadic version of this function so we can call `ProhibitAttribtues(CXX11Attrs, GNUAttrs);` or if that's just overkill. ================ Comment at: clang/lib/Parse/ParseStmt.cpp:340 case tok::kw___try: - ProhibitAttributes(Attrs); // TODO: is it correct? + ProhibitAttributes(CXX11Attrs); // TODO: is it correct? + ProhibitAttributes(GNUAttrs); ---------------- You can remove the TODO, it is correct: https://godbolt.org/z/PEcarx1nr ================ 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, Attrs); return Actions.ConvertDeclToDeclGroup(TheDecl); ---------------- rsmith wrote: > We should `ProhibitAttrs` here rather than passing them on. > ``` > [[]] extern "C" void f(); > ``` > ... is invalid. (Per the grammar in https://eel.is/c++draft/dcl.dcl#dcl.pre-1 > and https://eel.is/c++draft/dcl.dcl#dcl.link-2 an attribute-specifier-seq > can't appear here.) +1, looks like we're missing test coverage for that case (but those diagnostics by GCC or MSVC... hoo boy!): https://godbolt.org/z/cTfPbK837 ================ Comment at: clang/lib/Sema/ParsedAttr.cpp:219 + if (!isStandardAttributeSyntax()) + return true; + ---------------- rsmith wrote: > I think this case is unreachable, because only the standard `[[...]]` syntax > attributes are parsed as declaration attributes. I think it's unreachable today, but there's a nonzero chance for it to become reachable in the near future as WG21 and WG14 continue to contemplate adding standardized type attributes, so it seems reasonably forward-thinking to have it. ================ Comment at: clang/lib/Sema/ParsedAttr.cpp:224 + // property is consciously not defined as a flag in Attr.td because we don't + // want new attributes to specify it. + switch (getParsedKind()) { ---------------- Thoughts on the additional comment? And should we start to issue that deprecation warning now (perhaps as a separate follow-up commit)? ================ Comment at: clang/lib/Sema/ParsedAttr.cpp:306-307 + ParsedAttributes &Result) { + // Note that takeAllFrom() puts the attributes at the beginning of the list, + // so to obtain the correct ordering, we add `Second`, then `First`. + Result.takeAllFrom(Second); ---------------- rsmith wrote: > Are you sure about this? I looked at the implementation of `takeAllFrom` and > `takePool`, and it looks like it adds the new attributes to the end: > ``` > void AttributePool::takePool(AttributePool &pool) { > Attrs.insert(Attrs.end(), pool.Attrs.begin(), pool.Attrs.end()); > pool.Attrs.clear(); > } > ``` As mentioned earlier, we've got some preexisting ordering confusion somewhere in our attribute processing code, so I wouldn't be surprised if we're getting close to finding the root cause of that. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8361 + break; + if (AL.slidesFromDeclToDeclSpec()) { + if (AL.isStandardAttributeSyntax() && AL.isClangScope()) { ---------------- rsmith wrote: > Should we also be checking here that `D` is a declaration that the attribute > could have been moved onto -- that is, that it's a `DeclaratorDecl`? > Presumably we should error rather than only warning on > ``` > namespace N {} > [[clang::noderef]] using namespace N; > ``` We have `ParsedAttr::diagAppertainsToDecl()`, but that produces a diagnostic about the attribute not applying to the declaration which we wouldn't want. However, if we really wanted this, we could modify ClangAttrEmitter.cpp to produce a helper function for testing appertainment. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8367-8368 + // attributes might hurt portability. + S.Diag(AL.getLoc(), diag::warn_type_attribute_deprecated_on_decl) + << AL << D->getLocation(); + } ---------------- Do we have enough information at hand that we could produce a fix-it to move the attribute in question to the type position, or is that more work than it's worth? ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9118 + ProcessDeclAttributeOptions Options; + Options.IncludeCXX11Attributes = AL.isCXX11Attribute(); + ProcessDeclAttribute(*this, nullptr, ASDecl, AL, Options); ---------------- rsmith wrote: > This seems to be equivalent to setting `IncludeCXX11Attributes` to `true`, > which seems to be equivalent to not setting it at all. Hmmm, not quite -- `AL.isCXX11Attribute()` may return `false` (like for the GNU spelling of this attribute). ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9265 + S, D, PD.getDeclSpec().getAttributes(), + ProcessDeclAttributeOptions().WithIgnoreTypeAttributes(true)); + } ---------------- rsmith wrote: > I think we should be skipping C++11 attributes here too: `int [[x]] a;` > should not apply the attribute `x` to `a`, even if it's a declaration > attribute. +1 to that example being something we want to diagnose. ================ Comment at: clang/lib/Sema/SemaType.cpp:1826 + // attributes might hurt portability. + if (AL.isStandardAttributeSyntax() && AL.isClangScope()) { + S.Diag(AL.getLoc(), diag::warn_type_attribute_deprecated_on_decl) ---------------- Same question here as above on whether we can produce a fix-it here. 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-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits