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
