mboehme marked 6 inline comments as done. mboehme added a comment. Thank you @rsmith and @aaron.ballman for the detailed review comments!
I hope I've addressed everything -- do let me know if there's anything I've missed. In D126061#3528966 <https://reviews.llvm.org/D126061#3528966>, @rsmith wrote: > This direction looks good to me. Thanks! > > Regarding the possibility of storing the declaration attributes on the > `DeclSpec` rather than on the `Declarator`, I wonder if a better choice might > be to make the `Declarator` hold a non-owning reference to the attributes, > like it does for the `DeclSpec`? Thanks for the suggestion -- this has turned out to make the code simpler and more logical in a number of places. ================ Comment at: clang/include/clang/Sema/DeclSpec.h:1853-1856 /// Attrs - Attributes. ParsedAttributes Attrs; + ParsedAttributes DeclarationAttrs; ---------------- aaron.ballman wrote: > We should probably rename `Attrs` to be less generic and add some comments to > explain why there are two lists of parsed attributes. I've added some comments to clarify the difference. I'm not sure what a good alternative name for `Attrs` would be though. The only obvious candidate that comes to mind is `DeclaratorAttrs`, but that seems pretty redundant given that this is a field of `Declarator`. I think there is actually a good case for calling the first list simply `Attrs` because, unlike `DeclarationAttrs`, it applies directly to the `Declarator`. This distinction is amplified by the fact that, since your review, I've changed `DeclarationAttrs` to be a reference, not a by-value field. ================ Comment at: clang/include/clang/Sema/DeclSpec.h:1975-1984 /// Reset the contents of this Declarator. void clear() { + clearExceptDeclarationAttrs(); + DeclarationAttrs.clear(); + } + + /// Reset the contents of this Declarator, except for the declaration ---------------- rsmith wrote: > Note that neither of these functions clear the `DeclSpec`. I suppose that > makes sense given that the `Declarator` does not own the `DeclSpec` but > merely holds a reference to it. Perhaps we could do the same thing with the > declaration attributes? This is a great idea that also helps with other things. It's similar in spirit to the idea that I'd floated in our discussion on https://reviews.llvm.org/D111548 of introducing a `Declaration` and having `Declarator` hold a reference to that, but it seemed a bit excessive to introduce a new `Declaration` class solely for the purpose of holding the declaration attributes. Holding a reference directly to the declaration attributes achieves the same thing, but without having to introduce yet another class. I like it. One implication of this is that we should only have a const version of `getDeclarationAttributes()`; a non-const version would be dangerous because we're potentially sharing the attribute list with other declarators. (In fact, this wasn't really sound before either because we were effectively already sharing the attribute list too by moving it around between declarators.) Fortunately, it turns out that all of the places that were using the non-const `getDeclarationAttributes()` didn't actually need to be manipulating the declaration attribute list. I've added source code comments to call this out where appropriate. ================ 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, ---------------- aaron.ballman wrote: > 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. Yes, this was inteded to refer to the fact that non-[[]] attributes also notionally slide to the `DeclSpec`. In the initial version of my patch, it was possible for `slidesFromDeclToDeclSpec()` to be called on non-[[]] attributes within the default case in `ProcessDeclAttribute()`. However, I've realized that this is confusing; it's better to restrict this function only to [[]] attributes and to do an explicit check in `ProcessDeclAttribute()` to see whether we're looking at a [[]] attribute. I've added documentation to this function making it clear that it should only be called on [[]] attributes, and I've also added an assertion to enforce this. All other places that call `slidesFromDeclToDeclSpec()` are already guaranteed to only call it on [[]] attributes because those attributes come from `Declarator::getDeclarationAttributes()`, and that is only ever populated with [[]] attributes. (I've added an assertion to make sure this is and continues to be true.) ================ Comment at: clang/lib/Parse/ParseDecl.cpp:1832 // Parse the common declaration-specifiers piece. ParsingDeclSpec DS(*this); ---------------- rsmith wrote: > Ideally I think we should pass the `DeclSpecAttrs` into `DS` up-front here. > That'd keep the behavior closer to the behavior we get for attributes we > parse in the `ParseDeclarationSpecifiers` call below, and will keep > attributes in the correct order in the case of something like > `__attribute__((a)) int __attribute__((b)) x;` at block scope. Done. Unfortunately this did require adding the `OriginalDeclSpecAttrs` for the purpose of the `ProhibitAttributes()` call below. That call can't simply use `DS.getAttributes()` because `ParseDeclarationSpecifiers()` might have added attributes to that that are legitimately allowed to be there. For example, here's a case from test/SemaCXX/MicrosoftExtensions.cpp that would otherwise break: ``` // expected-warning@+1 {{attribute 'deprecated' is ignored, place it after "struct" to apply attribute to type declaration}} struct D {} __declspec(deprecated); ``` Interestingly, I believe the attribute ordering was already correct before this change because `DeclSpec::takeAttributesFrom()` adds attributes at the beginning of the list. (It may even be that it does this to support this very use case.) However, as we discuss elsewhere, this behavior is potentially surprising, and we may want to change it at some point, so it's better not to rely on it in the first place. ================ Comment at: clang/lib/Parse/ParseDecl.cpp:2188 // Parse the next declarator. - D.clear(); + D.clearExceptDeclarationAttrs(); D.setCommaLoc(CommaLoc); ---------------- aaron.ballman wrote: > 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). Now that `Declarator` only holds a reference to the declaration attributes, the additional `clearExceptDeclarationAttrs()` has become unnecessary. With that, I think the original `clear()` name is acceptable? ================ 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." ---------------- aaron.ballman wrote: > 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?) > }; > ``` Good point. Test added in test/Parser/c2x-attributes.c. ================ Comment at: clang/lib/Parse/ParseDecl.cpp:4385 + // can put them on the next field. + Attrs.takeAllFrom(DeclaratorInfo.D.getDeclarationAttributes()); } ---------------- rsmith wrote: > I think we'd benefit here from the `Declarator` only holding a reference to > the declaration attributes rather than owning them. Agree -- this has made this function much more straightforward. ================ 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. ---------------- aaron.ballman wrote: > 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! Thank you -- done! ================ Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2716 // Otherwise, it must be a using-declaration or an alias-declaration. return ParseUsingDeclaration(DeclaratorContext::Member, TemplateInfo, + UsingLoc, DeclEnd, DeclAttrs, AS); ---------------- rsmith wrote: > Do we need to `ProhibitAttrs(DeclSpecAttrs)` along this code path? For > example: > ``` > struct A { int a; }; > struct B : A { [uuid("1234")] using A::a; }; > ``` > ... currently warns (for `-target x86_64-windows`) but I think with this > patch we'll silently drop the `uuid` attribute on the floor. Good point. Instead of doing a `ProhibitAttributes()`, I've decided to simply move the `MaybeParseMicrosoftAttributes()` down below this point -- that seemed more straightforward. I've added a test in test/Parser/MicrosoftExtensions.cpp. Some considerations: - With this patch, we've "upgraded" the warning to a hard error. I think this is defensible though because b) the Microsoft compiler also flags this as a hard error (https://godbolt.org/z/jrx6Y1Y83), b) most people will be using Clang to compile code that they compiled with MSVC originally or in parallel, and c) it's an error that I think people are unlikely to make in the first place, - The error message we get isn't great, but it's not really worse than the Microsoft compiler's error message, and as noted above, this is an error that I think people are unlikely to make. ================ Comment at: clang/lib/Parse/ParseObjc.cpp:661 allTUVariables.push_back( - ParseDeclaration(DeclaratorContext::File, DeclEnd, attrs)); + ParseDeclaration(DeclaratorContext::File, DeclEnd, attrs, attrs)); continue; ---------------- rsmith wrote: > It's a bit confusing to use the same `attrs` variable twice here. Would be > clearer if either `attrs` were renamed to `NoAttrs` or if you used two > different variables. And... actually, given that `ParseDeclaration` takes > *mutable* references to its attribute lists, I'm pretty concerned by passing > in two different mutable references to the same variable -- that seems > error-prone. > > But see comments elsewhere; I think we can remove the `DeclSpecAttrs` > parameter from `ParseDeclaration` entirely. Good points -- I've used two separate variables. ================ 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()); ---------------- aaron.ballman wrote: > 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). As it turns out, we can never pass declaration attributes in here anyway -- see the newly added assertion at the top of the function. This is a good thing, because I've eliminated the non-const version of `Declarator::getDeclarationAttributes()`. ================ Comment at: clang/lib/Parse/ParseStmt.cpp:198 + ParsedAttributes Attrs(AttrFactory); + ConcatenateAttributes(CXX11Attrs, GNUAttrs, Attrs); + ---------------- aaron.ballman wrote: > 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. Thanks for pointing this out. I agree we shouldn't do this sorting unless it turns out to be necessary. ================ 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); ---------------- aaron.ballman wrote: > 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 I investigated this, but I'm not sure it's a net win. > * If the next token is `kw_static_assert`, `kw_using`, or `kw_namespace`, > then prohibit attributes and call `ParseDeclaration`. Or if the next token is `kw_inline` and the token after that is `kw_namespace`... I think going down this path would mean duplicating all of the case distinctions in `ParseDeclaration()` -- and if we add more cases in the future, we'd have to make sure to replicate them here. I think overall it keeps the code simpler to accept the additional `DeclSpecAttrs` parameter. > Do we also have to handle opaque-enum-declaration here? > http://eel.is/c++draft/dcl.dcl#nt:block-declaration A moot point, probably, but I believe this is handled by `ParseEnumSpecifier()`, which is called from `ParseDeclarationSpecifiers()`, so there would be no need to handle it separately. ================ Comment at: clang/lib/Parse/ParseStmt.cpp:317-318 case tok::kw_asm: { - ProhibitAttributes(Attrs); + ProhibitAttributes(CXX11Attrs); + ProhibitAttributes(GNUAttrs); bool msAsm = false; ---------------- aaron.ballman wrote: > It seems like we might have a pre-existing bug here for [[]] attributes? > http://eel.is/c++draft/dcl.dcl#nt:asm-declaration Yes, we do! Fixed, and tests added in test/Parser/asm.c and test/Parser/asm.cpp. ================ Comment at: clang/lib/Parse/ParseStmt.cpp:329-330 case tok::kw___if_not_exists: - ProhibitAttributes(Attrs); + ProhibitAttributes(CXX11Attrs); + ProhibitAttributes(GNUAttrs); ParseMicrosoftIfExistsStatement(Stmts); ---------------- aaron.ballman wrote: > 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. I would lean towards keeping it simple. Duplicating the `ProhibitAttributes()` is more verbose, but it's also less surprising. ================ Comment at: clang/lib/Parse/Parser.cpp:935-937 + ParsedAttributes DeclSpecAttrs(AttrFactory); + return ParseDeclaration(DeclaratorContext::File, DeclEnd, Attrs, + DeclSpecAttrs); ---------------- rsmith wrote: > For consistency with the below cases, call this one `EmptyDeclSpecAttrs` too Good point, done! ================ 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); ---------------- aaron.ballman wrote: > 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 Fixed and added a test in test/Parser/cxx0x-attributes.cpp. ================ Comment at: clang/lib/Sema/ParsedAttr.cpp:219 + if (!isStandardAttributeSyntax()) + return true; + ---------------- aaron.ballman wrote: > 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. As explained elsewhere, in the first iteration of the change, we used to actually call this with non-[[]] attributes. I've changed this now, and there's an assertion that this will only ever be called with [[]] attributes. ================ 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()) { ---------------- aaron.ballman wrote: > Thoughts on the additional comment? And should we start to issue that > deprecation warning now (perhaps as a separate follow-up commit)? > Thoughts on the additional comment? Yes, good point, we should point this out explicitly. I've worded the comment slightly differently: we can definitely deprecate the "sliding" behavior for attributes in the `clang::` namespace, as we own them, but there may be compatibility concerns for other attributes. This may mean that we can never reduce this list to nothing, but we should try. > And should we start to issue that deprecation warning now (perhaps as a > separate follow-up commit)? We're already doing this (see the tests in test/SemaCXX/adress-space-placement.cpp), though again only for attributes in the `clang::` namespace, as we don't really have the authority to deprecate this usage for other attributes. I think the other question here is whether, by default, this should be an error, not a warning, but this would then presumably require a command-line flag to downgrade the error to a warning if needed (I believe @rsmith raised this point on https://reviews.llvm.org/D111548). If we do decide to make this an error by default, I'd like to do this in a followup change if possible, as a) this change is already a strict improvement over the status quo; b) adding a command-line flag with associated tests would further increase the scope of this change, and c) this change is a blocker for https://reviews.llvm.org/D111548, which others on my team are waiting to be able to use. ================ 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); ---------------- aaron.ballman wrote: > 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. > 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(); > } > ``` This is the `AttributePool`, however, and IIUC that only deals with memory management for attributes; the ordering of attributes there should not have any semantic implications I believe. What's actually relevant for the semantic ordering of attributes is the call to `addAll()` in `takeAllFrom()`: ``` void takeAllFrom(ParsedAttributes &Other) { assert(&Other != this && "ParsedAttributes can't take attributes from itself"); addAll(Other.begin(), Other.end()); Other.clearListOnly(); pool.takeAllFrom(Other.pool); } ``` And `addAll()` inserts at the beginning of the list: ``` void addAll(iterator B, iterator E) { AttrList.insert(AttrList.begin(), B.I, E.I); } ``` The behavior is definitely surprising though. I speculate that it _may_ have been introduced to make `ParseSimpleDeclaration()` do the right thing (see my comments there). > 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. It's possible that other callers of `takeAllFrom()` or `addAll()` are expecting the attributes to get added at the end and are doing the wrong thing because of that. This seems worth investigating further, but I'd prefer to defer that to a followup change. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8361 + break; + if (AL.slidesFromDeclToDeclSpec()) { + if (AL.isStandardAttributeSyntax() && AL.isClangScope()) { ---------------- aaron.ballman wrote: > 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. Good point. I've added a check that we're looking at either a `DeclaratorDecl` or a `TypeAliasDecl`. The latter is necessary because we have existing use cases where attributes can be placed on an alias declaration and is interpreted as appertaining to the type; see this example from test/Parser/cxx0x-attributes.cpp: ``` using V = int; // expected-note {{previous}} using V [[gnu::vector_size(16)]] = int; // expected-error {{redefinition with different types}} ``` The various tests in test/SemaCXX/address-space-placement.cpp now reflect our rules for where the attribute exhibits the legacy "sliding" behavior (with a mere warning) and where it is prohibited outright (see the `expected-error` cases in that test). > However, if we really wanted this, we could modify ClangAttrEmitter.cpp to > produce a helper function for testing appertainment. That's a good point. I think for the time being what we have here is sufficient however. ================ 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(); + } ---------------- aaron.ballman wrote: > 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? I thought about this, but I think it would be non-trivial and would prefer to leave it to a future enhancement. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9118 + ProcessDeclAttributeOptions Options; + Options.IncludeCXX11Attributes = AL.isCXX11Attribute(); + ProcessDeclAttribute(*this, nullptr, ASDecl, AL, Options); ---------------- aaron.ballman wrote: > rsmith wrote: > > aaron.ballman wrote: > > > 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). > > It might, but we only care about the value of `IncludeCXX11Attributes` when > > processing a C++11 attribute, so it doesn't matter how this flag is set for > > a non-C++11 attribute. > Oh, good observation, you're right! Thanks for pointing this out -- done! ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9265 + S, D, PD.getDeclSpec().getAttributes(), + ProcessDeclAttributeOptions().WithIgnoreTypeAttributes(true)); + } ---------------- aaron.ballman wrote: > 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. > 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. Good point -- done. I don't think this will ultimately make much of a difference because we shouldn't be putting C++11 declaration attributes on the `DeclSpec` anyway; there's code in `ParseDeclarationSpecifiers()` that prohibits that. (Prior to this change, we were prohibiting C++11 attributes on the `DeclSpec` outright; now, we only forbid non-type attributes, but that does mean we should never see declaration attributes here.) There are existing tests for this, for example in test/Parser/cxx0x-attributes.cpp (search for "attribute cannot be applied to a declaration). However, it does seem more logical and consistent to exclude C++11 attributes here, so I've made this change. One effect that this does have is the treatment of unknown C++11 attributes: Because we're excluding them now, we no longer diagnose them here; instead, I've modified `processTypeAttrs()` to diagnose unknown C++11 attributes on the `DeclSpec` there. Again, this seems the more logical way to do things: The attributes appertain to the type, so they should be diagnosed as part of processing type attributes. ================ Comment at: clang/lib/Sema/SemaType.cpp:201 + // Set to indicate that, if we're currently processing the DeclSpec, the + // attributes we're seeing were actually written ahead of the declaration. + bool isProcessingDeclarationAttrs = false; ---------------- rsmith wrote: > It's not immediately clear to me which side is "ahead of"; "before" or > "after" would be clearer. No longer applicable. I've reverted the changes I made to `TypeProcessingState` that this comment applied to. I had originally modified `TypeProcessingState::getCurrentAttributes()` so that it would return the declaration attributes if we were currently processing those, but it turned out that this was not relevant to any of the use cases that `TypeProcessingState::getCurrentAttributes()` is used for. ================ Comment at: clang/lib/Sema/SemaType.cpp:593 state.addIgnoredTypeAttr(attr); -} + } ---------------- rsmith wrote: > Phabricator shows this line as slightly over-indented, but that might just be > phabricator being weird? This was actually a mis-formatting that clang-format didn't complain about or fix. Fixed. ================ 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) ---------------- aaron.ballman wrote: > Same question here as above on whether we can produce a fix-it here. Again, I would prefer to exclude this from the scope of this change. 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
