rsmith added inline comments.
================
Comment at: clang/include/clang/Sema/DeclSpec.h:1926-1928
+ assert(llvm::all_of(DeclarationAttrs, [](const ParsedAttr &AL) {
+ return AL.isStandardAttributeSyntax();
+ }));
----------------
I think I recall seeing OpenMP pragma attributes being parsed as declaration
attributes too. FIXME update this comment after review is complete
================
Comment at: clang/lib/Parse/ParseDecl.cpp:1854
+ ProhibitAttributes(DeclAttrs);
+ ProhibitAttributes(OriginalDeclSpecAttrs);
DeclEnd = Tok.getLocation();
----------------
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.
================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2691-2695
+ // We need to keep these attributes for future diagnostics
+ // before they are taken over by the declaration.
+ ParsedAttributesView FnAttrs;
+ FnAttrs.addAll(DeclAttrs.begin(), DeclAttrs.end());
+ FnAttrs.Range = DeclAttrs.Range;
----------------
Oh, neat, I think this copy isn't necessary any more, given that we don't give
ownership of the `DeclAttrs` to the `ParsingDeclarator` any more, and don't mix
the declaration attributes and the `DeclSpec` attributes together any more. We
should be able to just use `DeclAttrs` instead of `FnAttrs` below now, I think?
================
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);
----------------
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.
================
Comment at: clang/lib/Parse/ParseStmt.cpp:16
#include "clang/Basic/Attributes.h"
+#include "clang/Basic/DiagnosticSema.h"
#include "clang/Basic/PrettyStackTrace.h"
----------------
This header is intended to be private to Sema. If you want to use a diagnostic
in both the parser and sema, please move it to `DiagnosticCommonKinds.td`.
================
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);
----------------
mboehme wrote:
> 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.
> > * 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`...
This function is parsing block declarations, so we don't have to handle `inline
namespace` here because that can't appear as a block declaration. The only
reason we need to handle `namespace` is to support namespace aliases
(`namespace A = B;`), which are (perhaps surprisingly) allowed at block scope.
> 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.
It's not all the cases, only the block-declaration cases. If we wanted to
directly follow the grammar, we could split a `ParseBlockDeclaration` function
out of `ParseDeclaration`, but I guess there'd be so little left in
`ParseDeclaration` that it wouldn't be worth it, and we'd still have to pass
the `DeclSpecAttrs` parameter to `ParseBlockDeclaration`, so that doesn't save
us anything. I suppose it'd also regress our diagnostic quality for
declarations that aren't allowed at block scope. OK, I'm convinced :-)
================
Comment at: clang/lib/Sema/ParsedAttr.cpp:240-243
+ case AT_Regparm:
+ case AT_NoDeref:
+ case AT_ObjCGC:
+ case AT_VectorSize:
----------------
Many of these were previously not permitted in type attribute position, despite
being declared in Attr.td as `TypeAttr`s. Do they actually work in type
attribute position after this patch? At least `matrix_type` currently expects
to be applied only to a typedef declaration, so I'd expect that one does not
work as a type attribute.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8325-8326
+ // ParsedAttr::isCXX11Attribute() as the latter includes the alignas
+ // attribute. The alignas attribute is a declaration attribute but can
appear
+ // on the `DeclSpec`, so we want to let it through here.
+ if (AL.getSyntax() == ParsedAttr::AS_CXX11 &&
!Options.IncludeCXX11Attributes)
----------------
I'm not following what's going on here: the C++11 `alignas` keyword follows the
same rules as a C++11-syntax declaration attribute, so I'd expect it to be
handled the same way. Eg, `int alignas(16) n;` is ill-formed (even though ICC
and MSVC allow it and GCC only warns).
================
Comment at: clang/test/Parser/MicrosoftExtensions.cpp:65
+ // Can't apply a UUID to a using declaration.
+ [uuid("000000A0-0000-0000-C000-00000000004A")] using base::a; //
expected-error {{expected member name}}
+};
----------------
This seems like a confusing diagnostic to produce for this error. Does it make
sense in context (with the snippet + caret)?
================
Comment at: clang/test/SemaCXX/address-space-placement.cpp:3-5
+// Check that we emit the correct warnings in various situations where the
C++11
+// spelling of the `address_space` attribute is applied to a declaration
instead
+// of a type.
----------------
I'd like to see some matching tests that we *don't* warn when the attribute is
placed properly.
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
