rsmith added a comment.
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`?
================
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
----------------
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?
================
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,
----------------
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.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:1832
// Parse the common declaration-specifiers piece.
ParsingDeclSpec DS(*this);
----------------
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.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:2188
// Parse the next declarator.
- D.clear();
+ D.clearExceptDeclarationAttrs();
D.setCommaLoc(CommaLoc);
----------------
I wonder if a name like `prepareForNextDeclarator` or `clearForComma` would be
better here -- something that indicates why we're clearing rather than
describing how.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:4385
+ // can put them on the next field.
+ Attrs.takeAllFrom(DeclaratorInfo.D.getDeclarationAttributes());
}
----------------
I think we'd benefit here from the `Declarator` only holding a reference to the
declaration attributes rather than owning them.
================
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.
----------------
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).
================
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);
----------------
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.
================
Comment at: clang/lib/Parse/ParseObjc.cpp:661
allTUVariables.push_back(
- ParseDeclaration(DeclaratorContext::File, DeclEnd, attrs));
+ ParseDeclaration(DeclaratorContext::File, DeclEnd, attrs, attrs));
continue;
----------------
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.
================
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());
----------------
I think we should keep the attributes in appearance order.
================
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);
----------------
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.
================
Comment at: clang/lib/Parse/Parser.cpp:743
///
+/// The `Attrs that are passed in are C++11 attributes and appertain to the
+/// declaration.
----------------
error: unmatched `
================
Comment at: clang/lib/Parse/Parser.cpp:935-937
+ ParsedAttributes DeclSpecAttrs(AttrFactory);
+ return ParseDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
+ DeclSpecAttrs);
----------------
For consistency with the below cases, call this one `EmptyDeclSpecAttrs` too
================
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);
----------------
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.)
================
Comment at: clang/lib/Sema/ParsedAttr.cpp:219
+ if (!isStandardAttributeSyntax())
+ return true;
+
----------------
I think this case is unreachable, because only the standard `[[...]]` syntax
attributes are parsed as declaration attributes.
================
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);
----------------
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();
}
```
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8361
+ break;
+ if (AL.slidesFromDeclToDeclSpec()) {
+ if (AL.isStandardAttributeSyntax() && AL.isClangScope()) {
----------------
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;
```
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9118
+ ProcessDeclAttributeOptions Options;
+ Options.IncludeCXX11Attributes = AL.isCXX11Attribute();
+ ProcessDeclAttribute(*this, nullptr, ASDecl, AL, Options);
----------------
This seems to be equivalent to setting `IncludeCXX11Attributes` to `true`,
which seems to be equivalent to not setting it at all.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9152-9153
void Sema::checkUnusedDeclAttributes(Declarator &D) {
::checkUnusedDeclAttributes(*this, D.getDeclSpec().getAttributes());
+ ::checkUnusedDeclAttributes(*this, D.getDeclarationAttributes());
::checkUnusedDeclAttributes(*this, D.getAttributes());
----------------
May as well process these in lexical order.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9265
+ S, D, PD.getDeclSpec().getAttributes(),
+ ProcessDeclAttributeOptions().WithIgnoreTypeAttributes(true));
+ }
----------------
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.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9283-9289
+ ParsedAttributesView NonSlidingAttrs;
+ for (ParsedAttr &AL : PD.getDeclarationAttributes()) {
+ if (!AL.slidesFromDeclToDeclSpec()) {
+ NonSlidingAttrs.addAtEnd(&AL);
+ }
+ }
+ ProcessDeclAttributeList(S, D, NonSlidingAttrs);
----------------
These attributes should be processed first, so that we handle attributes in
appearance order.
================
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;
----------------
It's not immediately clear to me which side is "ahead of"; "before" or "after"
would be clearer.
================
Comment at: clang/lib/Sema/SemaType.cpp:593
state.addIgnoredTypeAttr(attr);
-}
+ }
----------------
Phabricator shows this line as slightly over-indented, but that might just be
phabricator being weird?
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
