aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Sema/DeclSpec.h:1853-1856
/// Attrs - Attributes.
ParsedAttributes Attrs;
+ ParsedAttributes DeclarationAttrs;
----------------
mboehme wrote:
> 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.
The new comment actually makes it less clear for me -- that says the attributes
are for the `DeclSpec` but they're actually for the `Declatator` (there's no
relationship between `Attrs` and `DS`).
Given that `DeclSpec` uses `Attrs` for its associated attributes, I think it's
okay to use `Attrs` here for the ones associated with the `Declarator`, but the
comment should be updated in that case.
================
Comment at: clang/include/clang/Sema/ParsedAttr.h:664
+ /// This may only be called if isStandardAttributeSyntax() returns true.
+ bool slidesFromDeclToDeclSpec() const;
+
----------------
Because this is now specific to standard attributes, and those don't "slide"
under normal circumstances, it might be nice to rename this method to make it
more clear that this should not be called normally. I don't have a strong
opinion on what a *good* name is, but even something like
`improperlySlidesFromDeclToDeclSpec()` would make me happy.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:1854
+ ProhibitAttributes(DeclAttrs);
+ ProhibitAttributes(OriginalDeclSpecAttrs);
DeclEnd = Tok.getLocation();
----------------
rsmith wrote:
> This looks wrong to me (both before and after this patch; you've faithfully
> retained an incorrect behavior). If `DeclSpec` attributes aren't allowed,
> they should be diagnosed by `ParsedFreeStandingDeclSpec`. Before and after
> this patch, we'll diagnose attributes in a free-standing decl-specifier-seq
> if we happened to tentatively parse them, not if we happened to parse them in
> `ParseDeclarationSpecifiers`. For example:
>
> ```
> __attribute__(()) struct X; // DeclSpecAttrs will be empty, no error
> void f() {
> __attribute(()) struct X; // DeclSpecAttrs will contain the attribute
> specifier, error
> }
> ```
>
> Comparing to GCC's behavior for that example (which GCC silently accepts) and
> for this one:
>
> ```
> __attribute__((packed)) struct X; // GCC warns that packed has no meaning
> here, clang produces a high-quality warning.
> void f() {
> __attribute((packed)) struct X; // GCC warns that packed has no meaning
> here, clang produces a bogus error.
> }
> ```
>
> ... I think the right thing to do is to delete this call (along with
> `OriginalDeclSpecAttrs`). GNU decl-specifier attributes *are* apparently
> syntactically permitted here, but most (maybe even all?) GNU attributes don't
> have any meaning in this position.
Good catch!
================
Comment at: clang/lib/Parse/ParseDecl.cpp:2188
// Parse the next declarator.
- D.clear();
+ D.clearExceptDeclarationAttrs();
D.setCommaLoc(CommaLoc);
----------------
mboehme wrote:
> 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?
It's fine by me.
================
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);
----------------
mboehme wrote:
> 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.
>
>
I think it's okay for this to have gone from a warning to a hard error,
especially given the MSVC compatibility.
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:2745-2746
// ptr-operators.
- Declarator D(DS, DeclaratorContext::ConversionId);
+ ParsedAttributes EmptyDeclAttrs(AttrFactory);
+ Declarator D(DS, EmptyDeclAttrs, DeclaratorContext::ConversionId);
ParseDeclaratorInternal(D, /*DirectDeclParser=*/nullptr);
----------------
rsmith wrote:
> Given the number of times this has come up, I wonder if it's worth adding
> something like:
>
> ```
> class ParsedAttributes {
> public:
> // ...
> static const ParsedAttributes &none() {
> static AttributeFactory Factory;
> static AttributePool Pool(Factory);
> static const ParsedAttributes Attrs(Pool);
> return Attrs;
> }
> // ...
> };
> ```
> (or, better, changing `Declarator` to hold a `const ParsedAttributesView&`
> rather than a `const ParsedAttributes&`) so that we can write
> ```
> Declarator D(DS, ParsedAttributes::none(), DeclaratorContext::ConversionId);
> ```
>
> Totally optional, though, and I'm happy for this cleanup to happen at some
> later point in time.
+1 to this being a pretty nice cleanup and it being fine if it happens in a
follow-up.
================
Comment at: clang/lib/Parse/ParseStmt.cpp:317-318
case tok::kw_asm: {
- ProhibitAttributes(Attrs);
+ ProhibitAttributes(CXX11Attrs);
+ ProhibitAttributes(GNUAttrs);
bool msAsm = false;
----------------
mboehme wrote:
> 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.
I think we should prohibit this for all `[[]]` attributes. C doesn't have an
asm statement as part of the standard, but it lists it as a common extension in
Annex J and uses the same syntax as the C++ feature. Might as well keep it
consistent between the languages.
================
Comment at: clang/lib/Parse/ParseStmt.cpp:329-330
case tok::kw___if_not_exists:
- ProhibitAttributes(Attrs);
+ ProhibitAttributes(CXX11Attrs);
+ ProhibitAttributes(GNUAttrs);
ParseMicrosoftIfExistsStatement(Stmts);
----------------
mboehme wrote:
> 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.
I don't feel super strongly about it, so this can wait until a follow-up if we
want to do anything about 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()) {
----------------
mboehme wrote:
> 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.
> ... 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.
I would be pretty aggressive there and break compatibility over this, unless
the uses were in system headers or something where the break would be too
onerous to justify.
> 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
You put `DefaultError` onto the warning diagnostic to have it automatically
upgraded as if the user did `-Werror=whatever-warning`, which allows the user
to downgrade it back into a warning with `-Wno-error=whatever-warning`.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8381
+ if (AL.slidesFromDeclToDeclSpec() &&
+ (clang::isa<DeclaratorDecl>(D) || clang::isa<TypeAliasDecl>(D))) {
+ // Suggest moving the attribute to the type instead, but only for our
----------------
`isa` is variadic.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8385
+ // portability.
+ if (AL.isClangScope()) {
+ S.Diag(AL.getLoc(), diag::warn_type_attribute_deprecated_on_decl)
----------------
I do wonder how much code we break if we tighten this up regardless of which
vendor namespace the attribute belongs to. I believe the sliding behavior is
not conforming behavior unless we issue a diagnostic about it because we're
extending the behavior of the language.
================
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();
+ }
----------------
mboehme wrote:
> 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.
Fine by me.
================
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)
----------------
mboehme wrote:
> 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.
SGTM!
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
