aaron.ballman added a comment.
I really only had one salient question (and a tiny nit), so I think this is
almost good to go.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:6501
+
+The attribute does not have any effect on the semantics of C++ code, neither
+type checking rules, nor runtime semantics. In particular:
----------------
(Since this is for C as well as C++)
================
Comment at: clang/lib/Parse/ParseDecl.cpp:3187-3193
+ // We reject AT_LifetimeBound and AT_AnyX86NoCfCheck, even though
they
+ // are type attributes, because we historically haven't allowed these
+ // to be used as type attributes in C++11 / C2x syntax.
+ if (PA.isTypeAttr() && PA.getKind() != ParsedAttr::AT_LifetimeBound
&&
+ PA.getKind() != ParsedAttr::AT_AnyX86NoCfCheck)
+ continue;
+ Diag(PA.getLoc(), diag::err_attribute_not_type_attr) << PA;
----------------
Should this be done as part of D126061 instead?
================
Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {
----------------
mboehme wrote:
> mboehme wrote:
> > mboehme wrote:
> > > mboehme wrote:
> > > > rsmith wrote:
> > > > > mboehme wrote:
> > > > > > rsmith wrote:
> > > > > > > mboehme wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > mboehme wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > > > > Can you also add some test coverage for
> > > > > > > > > > > > > > > > > > > applying the attribute to a declaration
> > > > > > > > > > > > > > > > > > > instead of a type or not giving it any
> > > > > > > > > > > > > > > > > > > arguments? Also, should test arguments
> > > > > > > > > > > > > > > > > > > which are not a constant expression.
> > > > > > > > > > > > > > > > > > I've added tests as you suggested, though I
> > > > > > > > > > > > > > > > > > put most of them in Sema/annotate-type.c,
> > > > > > > > > > > > > > > > > > as they're not specific to C++.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Thanks for prompting me to do this -- the
> > > > > > > > > > > > > > > > > > tests caused me to notice and fix a number
> > > > > > > > > > > > > > > > > > of bugs.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > I haven't managed to diagnose the case when
> > > > > > > > > > > > > > > > > > the attribute appears at the beginning of
> > > > > > > > > > > > > > > > > > the declaration. It seems to me that, at
> > > > > > > > > > > > > > > > > > the point where I've added the check, this
> > > > > > > > > > > > > > > > > > case is indistinguishable from an attribute
> > > > > > > > > > > > > > > > > > that appears on the type. In both cases,
> > > > > > > > > > > > > > > > > > the `TAL` is `TAL_DeclSpec`, and the
> > > > > > > > > > > > > > > > > > attribute is contained in
> > > > > > > > > > > > > > > > > > `DeclSpec::getAttributes()`. This is
> > > > > > > > > > > > > > > > > > because `Parser::ParseSimpleDeclaration`
> > > > > > > > > > > > > > > > > > forwards the declaration attributes to the
> > > > > > > > > > > > > > > > > > `DeclSpec`:
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > I believe if I wanted to prohibit this
> > > > > > > > > > > > > > > > > > case, I would need to add a check to
> > > > > > > > > > > > > > > > > > `Parser::ParseStatementOrDeclaration` and
> > > > > > > > > > > > > > > > > > prohibit any occurrences of `annotate_type`
> > > > > > > > > > > > > > > > > > there. However, this seems the wrong place
> > > > > > > > > > > > > > > > > > to do this because it doesn't contain any
> > > > > > > > > > > > > > > > > > specific processing for other attributes.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > I've noticed that Clang also doesn't
> > > > > > > > > > > > > > > > > > prohibit other type attributes (even ones
> > > > > > > > > > > > > > > > > > with C++ 11 syntax) from being applied to
> > > > > > > > > > > > > > > > > > declarations. For example:
> > > > > > > > > > > > > > > > > > https://godbolt.org/z/Yj1zWY7nn
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > How do you think I should proceed here? I
> > > > > > > > > > > > > > > > > > think the underlying issue is that Clang
> > > > > > > > > > > > > > > > > > doesn't always distinguish cleanly between
> > > > > > > > > > > > > > > > > > declaration attributes and type attributes,
> > > > > > > > > > > > > > > > > > and fixing this might be nontrivial.
> > > > > > > > > > > > > > > > > > How do you think I should proceed here? I
> > > > > > > > > > > > > > > > > > think the underlying issue is that Clang
> > > > > > > > > > > > > > > > > > doesn't always distinguish cleanly between
> > > > > > > > > > > > > > > > > > declaration attributes and type attributes,
> > > > > > > > > > > > > > > > > > and fixing this might be nontrivial.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > This is a general issue with attribute
> > > > > > > > > > > > > > > > > processing. I would imagine that
> > > > > > > > > > > > > > > > > SemaDeclAttr.cpp should be able to diagnose
> > > > > > > > > > > > > > > > > that case when the attribute only applies to
> > > > > > > > > > > > > > > > > types and not declarations, but it'd take
> > > > > > > > > > > > > > > > > some investigation for me to be sure.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Because this issue isn't new to your
> > > > > > > > > > > > > > > > > situation, I'd recommend filing an issue
> > > > > > > > > > > > > > > > > about the general problem and then we can
> > > > > > > > > > > > > > > > > solve that later.
> > > > > > > > > > > > > > > > I've done some more investigation myself, and I
> > > > > > > > > > > > > > > > think I've come up with a solution; actually,
> > > > > > > > > > > > > > > > two potential solutions. I have draft patches
> > > > > > > > > > > > > > > > for both of these; I wanted to run these by you
> > > > > > > > > > > > > > > > here first, so I haven't opened a bug yet.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I'd appreciate your input on how you'd prefer
> > > > > > > > > > > > > > > > me to proceed with this. I do think it makes
> > > > > > > > > > > > > > > > sense to do this work now because otherwise,
> > > > > > > > > > > > > > > > people will start putting `annotate_type` in
> > > > > > > > > > > > > > > > places where it doesn't belong, and then we'll
> > > > > > > > > > > > > > > > have to keep supporting that in the future.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > **My preferred solution**
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > https://reviews.llvm.org/D124081
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > This adds a function
> > > > > > > > > > > > > > > > `DiagnoseCXX11NonDeclAttrs()` and calls it in
> > > > > > > > > > > > > > > > all places where we should reject attributes
> > > > > > > > > > > > > > > > that can't be put on declarations. (As part of
> > > > > > > > > > > > > > > > this work, I noticed that `gsl::suppress`
> > > > > > > > > > > > > > > > should be a `DeclOrStmtAttr`, not just a
> > > > > > > > > > > > > > > > `StmtAttr`.)
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Advantages:
> > > > > > > > > > > > > > > > - Not very invasive.
> > > > > > > > > > > > > > > > Disadvantages:
> > > > > > > > > > > > > > > > - Need to make sure we call
> > > > > > > > > > > > > > > > `DiagnoseCXX11NonDeclAttrs()` everywhere that
> > > > > > > > > > > > > > > > non-declaration attributes should be rejected.
> > > > > > > > > > > > > > > > (Not sure if I've identified all of those
> > > > > > > > > > > > > > > > places yet?)
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > **Alternative solution**
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > https://reviews.llvm.org/D124083
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > This adds an `appertainsTo` parameter to
> > > > > > > > > > > > > > > > `ParseCXX11Attributes()` and other "parse
> > > > > > > > > > > > > > > > attributes" functions that call it. This
> > > > > > > > > > > > > > > > parameter specifies whether the attributes in
> > > > > > > > > > > > > > > > the place where they're currently being parsed
> > > > > > > > > > > > > > > > appertain to a declaration, statement or type.
> > > > > > > > > > > > > > > > If `ParseCXX11Attributes()` encounters an
> > > > > > > > > > > > > > > > attribute that is not compatible with
> > > > > > > > > > > > > > > > `appertainsTo`, it outputs a diagnostic.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Advantages:
> > > > > > > > > > > > > > > > - Every call to `ParseCXX11Attributes()` _has_
> > > > > > > > > > > > > > > > to specify `appertainsTo` -- so there's no risk
> > > > > > > > > > > > > > > > of missing some case where we have to output
> > > > > > > > > > > > > > > > diagnostics
> > > > > > > > > > > > > > > > Disadvantages:
> > > > > > > > > > > > > > > > - This change is _much_ more invasive.
> > > > > > > > > > > > > > > > - It's not always clear what value to specify
> > > > > > > > > > > > > > > > for `appertainsTo`. The poster child for this
> > > > > > > > > > > > > > > > is `Parser::ParseStatementOrDeclaration()`: As
> > > > > > > > > > > > > > > > the name says, we don't yet know whether we're
> > > > > > > > > > > > > > > > parsing a statement or a declaration. As a
> > > > > > > > > > > > > > > > result, we need to be able to specify
> > > > > > > > > > > > > > > > `AppertainsToUnknown` and would need to add
> > > > > > > > > > > > > > > > additional logic deeper in the call stack to
> > > > > > > > > > > > > > > > produce diagnostics once we know for sure
> > > > > > > > > > > > > > > > whether we're dealing with a statement or
> > > > > > > > > > > > > > > > declaration. (This isn't yet implemented.) This
> > > > > > > > > > > > > > > > makes the solution less elegant that it
> > > > > > > > > > > > > > > > initially seems.
> > > > > > > > > > > > > > > > - There's a tension between this new
> > > > > > > > > > > > > > > > functionality and the existing
> > > > > > > > > > > > > > > > `ParsedAttr::diagnoseAppertainsTo()`. However,
> > > > > > > > > > > > > > > > we unfortunately can't extend the existing
> > > > > > > > > > > > > > > > `ParsedAttr::diagnoseAppertainsTo()` because it
> > > > > > > > > > > > > > > > is called from `ProcessDeclAttribute()` and
> > > > > > > > > > > > > > > > (despite the name) that function is also
> > > > > > > > > > > > > > > > processes some legitimate type attributes.
> > > > > > > > > > > > > > > > I do think it makes sense to do this work now
> > > > > > > > > > > > > > > > because otherwise, people will start putting
> > > > > > > > > > > > > > > > annotate_type in places where it doesn't
> > > > > > > > > > > > > > > > belong, and then we'll have to keep supporting
> > > > > > > > > > > > > > > > that in the future.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Thank you for working on this now, I agree that'd
> > > > > > > > > > > > > > > be good to avoid.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > My concern with both of those approaches is that
> > > > > > > > > > > > > > > they impact *parsing* rather than *semantics* and
> > > > > > > > > > > > > > > that seems wrong. As far as the parser is
> > > > > > > > > > > > > > > concerned, attributes really only exist as one
> > > > > > > > > > > > > > > particular form of syntax or another, but it
> > > > > > > > > > > > > > > shouldn't care about whether an attribute is
> > > > > > > > > > > > > > > allowed to appertain to a particular thing or
> > > > > > > > > > > > > > > not. e.g., an attribute list can either appear
> > > > > > > > > > > > > > > somewhere in the syntax or it can't, but what
> > > > > > > > > > > > > > > particular attributes are specified do not matter
> > > > > > > > > > > > > > > at parsing time. Parsing determine what the
> > > > > > > > > > > > > > > attribute is associated with (a declaration, a
> > > > > > > > > > > > > > > type, a statement, etc). After we've finished
> > > > > > > > > > > > > > > parsing the attributes in the list, we convert
> > > > > > > > > > > > > > > the attributes into their semantic form and
> > > > > > > > > > > > > > > that's the time at which we care about whether
> > > > > > > > > > > > > > > the attribute may be written on the thing it
> > > > > > > > > > > > > > > appertains to.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I think we handle all of the parsing correctly
> > > > > > > > > > > > > > > today already, but the issue is on the semantic
> > > > > > > > > > > > > > > side of things. An attribute at the start of a
> > > > > > > > > > > > > > > line can only be one of two things: a statement
> > > > > > > > > > > > > > > attribute or a declaration attribute, it can
> > > > > > > > > > > > > > > never be a type attribute. So what I think should
> > > > > > > > > > > > > > > happen is that `ProcessDeclAttribute()` needs to
> > > > > > > > > > > > > > > get smarter when it's given an attribute that can
> > > > > > > > > > > > > > > only appertain to a type. I don't think we should
> > > > > > > > > > > > > > > have to touch the parser at all, especially
> > > > > > > > > > > > > > > because you can specify multiple attributes in
> > > > > > > > > > > > > > > one list, as in:
> > > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > > void func() {
> > > > > > > > > > > > > > > [[decl_attr, type_attr]] int foo;
> > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > > and we'd want to diagnose `type_attr` but still
> > > > > > > > > > > > > > > allow `decl_attr`.
> > > > > > > > > > > > > > > I think we handle all of the parsing correctly
> > > > > > > > > > > > > > > today already
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I'm not sure about this. For example, take a look
> > > > > > > > > > > > > > at `Parser::ParseSimpleDeclaration`, which does
> > > > > > > > > > > > > > this at the end:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > DS.takeAttributesFrom(Attrs);
> > > > > > > > > > > > > > ```
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Here, `DS` is a `ParsingDeclSpec`, and `Attrs` are
> > > > > > > > > > > > > > the attributes that were seen on the declaration (I
> > > > > > > > > > > > > > believe).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > If my understanding of the code is correct (which
> > > > > > > > > > > > > > it very well may not be), this is putting the
> > > > > > > > > > > > > > attributes that were seen on the declaration in the
> > > > > > > > > > > > > > same place (`DeclSpec::Attrs`) that will later
> > > > > > > > > > > > > > receive the attributes seen on the
> > > > > > > > > > > > > > decl-specifier-seq. Once we've put them there, it
> > > > > > > > > > > > > > is no longer possible to tell whether those
> > > > > > > > > > > > > > attributes were seen on the declaration (where type
> > > > > > > > > > > > > > attributes are not allowed) or on the
> > > > > > > > > > > > > > decl-specifier-seq (where type attributes _are_
> > > > > > > > > > > > > > allowed).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > (This is why https://reviews.llvm.org/D124081 adds
> > > > > > > > > > > > > > a `DiagnoseCXX11NonDeclAttrs(Attrs)` just before
> > > > > > > > > > > > > > the `DS.takeAttributesFrom(Attrs)` call -- because
> > > > > > > > > > > > > > this is the last point at which we haven't lumped
> > > > > > > > > > > > > > declaration and decl-specifier-seq attributes
> > > > > > > > > > > > > > together.)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > `ProcessDeclAttribute()`, when called through
> > > > > > > > > > > > > > `Sema::ProcessDeclAttributes()` (which I think is
> > > > > > > > > > > > > > the relevant call stack), only sees attributes
> > > > > > > > > > > > > > after they have been put in `DeclSpec::Attrs`. (It
> > > > > > > > > > > > > > therefore sees, and ignores, all of the type
> > > > > > > > > > > > > > attributes that were put on the
> > > > > > > > > > > > > > decl-specifier-seq.) So I think it isn't possible
> > > > > > > > > > > > > > to tell there whether we put a type attribute on a
> > > > > > > > > > > > > > declaration.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > It might be possible to fix this by giving
> > > > > > > > > > > > > > `DeclSpec` two lists of attributes, one containing
> > > > > > > > > > > > > > the attributes for the declaration, and another
> > > > > > > > > > > > > > containing the attributes for the
> > > > > > > > > > > > > > decl-specifier-seq itself. However, this is a
> > > > > > > > > > > > > > pretty invasive change, as everything that consumes
> > > > > > > > > > > > > > attributes from the `DeclSpec` potentially needs to
> > > > > > > > > > > > > > be changed (I tried and shied away from it).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Am I misunderstanding something about the code? If
> > > > > > > > > > > > > > not, what are your thoughts on how best to proceed?
> > > > > > > > > > > > > > Am I misunderstanding something about the code? If
> > > > > > > > > > > > > > not, what are your thoughts on how best to proceed?
> > > > > > > > > > > > >
> > > > > > > > > > > > > I think your understanding is correct and I was
> > > > > > > > > > > > > imprecise. I meant that we generally parse the `[[]]`
> > > > > > > > > > > > > in all the expected locations in the parser. However,
> > > > > > > > > > > > > I think you're correct that the bits which determine
> > > > > > > > > > > > > what chunks in a declaration may need to be updated.
> > > > > > > > > > > > > That code largely exists because GNU-style attributes
> > > > > > > > > > > > > "slide" sometimes to other chunks, but that's not the
> > > > > > > > > > > > > case for [[]]-style attributes. It sounds like we are
> > > > > > > > > > > > > likely lacking expressivity for the `[[]]` type
> > > > > > > > > > > > > attributes in the hookup between what we parsed and
> > > > > > > > > > > > > when we form the semantic attribute because we need
> > > > > > > > > > > > > to convert the parsed type into a real type.
> > > > > > > > > > > > > It sounds like we are likely lacking expressivity for
> > > > > > > > > > > > > the [[]] type attributes in the hookup between what
> > > > > > > > > > > > > we parsed and when we form the semantic attribute
> > > > > > > > > > > > > because we need to convert the parsed type into a
> > > > > > > > > > > > > real type.
> > > > > > > > > > > >
> > > > > > > > > > > > OK, good, sounds like we're on the same page!
> > > > > > > > > > > >
> > > > > > > > > > > > I'll look at what we can do to retain the information
> > > > > > > > > > > > of whether an attribute was added to the declaration or
> > > > > > > > > > > > the decl-specifier-seq, so that Sema can then issue the
> > > > > > > > > > > > correct diagnostics. I'll look at changing `DeclSpec`
> > > > > > > > > > > > so that it retains not one, but two lists of
> > > > > > > > > > > > attributes, for the declaration and the
> > > > > > > > > > > > decl-specifier-seq respectively. There are likely to be
> > > > > > > > > > > > some devils in the details -- `DeclSpec` allows direct
> > > > > > > > > > > > write access to its `Attrs` member variable through the
> > > > > > > > > > > > non-const version of `getAttributes()`, and there are
> > > > > > > > > > > > quite a few places in the code that call this. Does
> > > > > > > > > > > > this seem like the right direction to you in principle
> > > > > > > > > > > > though?
> > > > > > > > > > > >
> > > > > > > > > > > > Apart from all of this, are there any outstanding
> > > > > > > > > > > > issues that you see with this change? It does of course
> > > > > > > > > > > > make sense to prohibit type attributes on declarations
> > > > > > > > > > > > before I land this change, but I wanted to check
> > > > > > > > > > > > whether there's anything else that would need to happen
> > > > > > > > > > > > here.
> > > > > > > > > > > > Does this seem like the right direction to you in
> > > > > > > > > > > > principle though?
> > > > > > > > > > >
> > > > > > > > > > > Maybe. In terms of the standards concepts, a
> > > > > > > > > > > `decl-specifier-seq` can have attributes associated with
> > > > > > > > > > > it
> > > > > > > > > > > (https://eel.is/c++draft/dcl.dcl#dcl.spec.general-1.sentence-2)
> > > > > > > > > > > and so from that perspective, a `DeclSpec` seems like it
> > > > > > > > > > > should have only one list of attributes (those for the
> > > > > > > > > > > entire sequence of declaration specifiers). However,
> > > > > > > > > > > GNU-style attributes "slide" around to other things, so
> > > > > > > > > > > it's possible that the behavior in
> > > > > > > > > > > `ParseSimpleDeclaration()` is correct for that particular
> > > > > > > > > > > syntax but incorrect for `[[]]` style attributes. So,
> > > > > > > > > > > depending on which devilish details are learned while
> > > > > > > > > > > working on this, it may turn out that the best approach
> > > > > > > > > > > is a secondary list, or it may turn out that we want to
> > > > > > > > > > > add a new `takeAll` variant that takes only the correct
> > > > > > > > > > > attributes while leaving others. (It may be useful to
> > > > > > > > > > > summon @rsmith for opinions on the right approach to take
> > > > > > > > > > > here.)
> > > > > > > > > > >
> > > > > > > > > > > > Apart from all of this, are there any outstanding
> > > > > > > > > > > > issues that you see with this change? It does of course
> > > > > > > > > > > > make sense to prohibit type attributes on declarations
> > > > > > > > > > > > before I land this change, but I wanted to check
> > > > > > > > > > > > whether there's anything else that would need to happen
> > > > > > > > > > > > here.
> > > > > > > > > > >
> > > > > > > > > > > Nothing else is jumping out at me as missing or needing
> > > > > > > > > > > additional work aside from some formatting nits. I'm
> > > > > > > > > > > going to add another reviewer just to make sure I've not
> > > > > > > > > > > missed something, but I think this is about ready to go.
> > > > > > > > > > Thanks for the input!
> > > > > > > > > >
> > > > > > > > > > I've looked at this more closely, and I've concluded that
> > > > > > > > > > adding a second list of attributes to the `DeclSpec` isn't
> > > > > > > > > > really the right way to go. For one thing, as you say, the
> > > > > > > > > > attributes we're discussing don't really belong on the
> > > > > > > > > > decl-specifier-seq.
> > > > > > > > > >
> > > > > > > > > > But I _have_ come up with an alternative solution that I
> > > > > > > > > > like better than the alternatives I had considered so far
> > > > > > > > > > and that, as you suggest, emits the diagnostics from Sema,
> > > > > > > > > > not the Parser. The solution is not just more concise than
> > > > > > > > > > the other options I had explored but also just feels like
> > > > > > > > > > the right way of doing things.
> > > > > > > > > >
> > > > > > > > > > Taking a look at the standard, attributes in the location
> > > > > > > > > > in question (at the beginning of a simple-declaration) have
> > > > > > > > > > the same semantics as attributes that appear after a
> > > > > > > > > > declarator-id, and we already have a location to store the
> > > > > > > > > > latter, namely `Declarator::Attr`. It therefore seems the
> > > > > > > > > > right thing to do also put attributes from the
> > > > > > > > > > simple-declaration in `Declarator::Attr`. We can then take
> > > > > > > > > > advantage of the fact that there is existing code already
> > > > > > > > > > in place to deal with attributes in this location.
> > > > > > > > > >
> > > > > > > > > > The other part of the solution is to make
> > > > > > > > > > `ProcessDeclAttribute` warn about C++11 attributes that
> > > > > > > > > > don’t “slide” to the DeclSpec (except, of course, if
> > > > > > > > > > `ProcessDeclAttribute` is actually being called for
> > > > > > > > > > attributes on a `DeclSpec` or `Declarator`).
> > > > > > > > > >
> > > > > > > > > > Here’s a draft change that implements this:
> > > > > > > > > >
> > > > > > > > > > https://reviews.llvm.org/D124919
> > > > > > > > > >
> > > > > > > > > > (The change description contains more details on the
> > > > > > > > > > attribute semantics from the standard that I alluded to
> > > > > > > > > > above.)
> > > > > > > > > >
> > > > > > > > > > The change still contains a few TODOs, but all of the hard
> > > > > > > > > > bits are done. Specifically, all of the tests pass too. I
> > > > > > > > > > feel pretty good about this change, but before I work on it
> > > > > > > > > > further to get it ready for review, I wanted to get your
> > > > > > > > > > reaction: Do you agree that this is the right general
> > > > > > > > > > direction?
> > > > > > > > > > The change still contains a few TODOs, but all of the hard
> > > > > > > > > > bits are done. Specifically, all of the tests pass too. I
> > > > > > > > > > feel pretty good about this change, but before I work on it
> > > > > > > > > > further to get it ready for review, I wanted to get your
> > > > > > > > > > reaction: Do you agree that this is the right general
> > > > > > > > > > direction?
> > > > > > > > >
> > > > > > > > > Thank you for your continued work on this, I *really*
> > > > > > > > > appreciate it! I think your newest approach is the one I'm
> > > > > > > > > happiest with, though there are still parts of it I hope we
> > > > > > > > > can find ways to improve. My biggest concern is with having
> > > > > > > > > to sprinkle `ExtractDefiniteDeclAttrs()` in the "correct"
> > > > > > > > > places, as that's likely to be fragile. If there was a way we
> > > > > > > > > only had to do that once instead of 8 times, I'd be much
> > > > > > > > > happier. My worry is mostly with someone forgetting to call
> > > > > > > > > it when they should or thinking they need to call it when
> > > > > > > > > modeling new code on existing code.
> > > > > > > > >
> > > > > > > > > Aside from that, I think the approach you've got is quite
> > > > > > > > > reasonable, though I'd be curious if @rsmith or @rjmccall
> > > > > > > > > have additional thoughts or concerns that I might not be
> > > > > > > > > seeing yet.
> > > > > > > > > My biggest concern is with having to sprinkle
> > > > > > > > > `ExtractDefiniteDeclAttrs()` in the "correct" places, as
> > > > > > > > > that's likely to be fragile. If there was a way we only had
> > > > > > > > > to do that once instead of 8 times, I'd be much happier. My
> > > > > > > > > worry is mostly with someone forgetting to call it when they
> > > > > > > > > should or thinking they need to call it when modeling new
> > > > > > > > > code on existing code.
> > > > > > > >
> > > > > > > > Agree. I would also prefer it if we could only do this in once
> > > > > > > > place -- but I can't really come up with a good way to do this.
> > > > > > > >
> > > > > > > > The underlying reason for why we're doing this in multiple
> > > > > > > > places is that there are multiple places in the C++ grammar
> > > > > > > > where an attribute-specifier-seq can occur that appertains to a
> > > > > > > > declaration (e.g. in a simple-declaration,
> > > > > > > > parameter-declaration, exception-declaration, and so on). These
> > > > > > > > are parsed in different places in the code, and so we also need
> > > > > > > > to add `ExtractDefiniteDeclAttrs()` in those different places.
> > > > > > > > If a future change to the language adds another type of
> > > > > > > > declaration that can contain an attribute-specifier-seq, we'll
> > > > > > > > have to add a new `ExtractDefiniteDeclAttrs()` there as well --
> > > > > > > > but that's really just a part of saying that we need to write
> > > > > > > > code that parses that entire type of declaration correctly.
> > > > > > > >
> > > > > > > > Unfortunately, I don't really see what we could do to make sure
> > > > > > > > people don't get this wrong. The motivation behind my previous
> > > > > > > > attempt in https://reviews.llvm.org/D124083 was to try and make
> > > > > > > > errors impossible by making callers of `ParseCXX11Attributes`
> > > > > > > > (and related functions) specify what entity (type, statement,
> > > > > > > > or declaration) the attributes appertain to in the current
> > > > > > > > position. Unfortunately, this can't be done consistently, as we
> > > > > > > > don't actually always know what entity the attributes should
> > > > > > > > appertain to (e.g. in `Parser::ParseStatementOrDeclaration`),
> > > > > > > > and the approach has other downsides, as we've discussed before.
> > > > > > > >
> > > > > > > > > Aside from that, I think the approach you've got is quite
> > > > > > > > > reasonable, though I'd be curious if @rsmith or @rjmccall
> > > > > > > > > have additional thoughts or concerns that I might not be
> > > > > > > > > seeing yet.
> > > > > > > >
> > > > > > > > I'd like to hear their input too! Besides your mentioning them
> > > > > > > > here, is there anything else we need to do to make sure they
> > > > > > > > see this? (Should I reach out to @rsmith internally?)
> > > > > > > >
> > > > > > > > By the way, https://reviews.llvm.org/D124919 currently depends
> > > > > > > > on this change (https://reviews.llvm.org/D111548), so that we
> > > > > > > > can show the effect that it has on the tests for
> > > > > > > > `annotate_type`, but if we agree that
> > > > > > > > https://reviews.llvm.org/D124919 is what we want to proceed
> > > > > > > > with, I would plan to submit it first so that we have the
> > > > > > > > "protections" it provides in place before adding the new
> > > > > > > > attribute.
> > > > > > > I think the right approach here is that `ParseDeclaration`,
> > > > > > > `ParseExternalDeclaration`, and friends should only be given an
> > > > > > > attribute list that includes declaration attributes (that is,
> > > > > > > C++11 attribute syntax attributes), and that list should not be
> > > > > > > muddled into the list of decl specifier attributes.
> > > > > > >
> > > > > > > As far as I can see, that is in fact already the case with only
> > > > > > > two exceptions;
> > > > > > >
> > > > > > > 1) `ParseExportDeclaration` parses MS attributes before recursing
> > > > > > > into `ParseExternalDeclaration`. That looks like it's simply a
> > > > > > > bug as far as I can tell, and that call can be removed. MS
> > > > > > > attributes will be parsed as part of the decl specifier sequence
> > > > > > > as needed and don't need to be parsed as declaration attributes.
> > > > > > > 2) `ParseStatementOrDeclarationAfterAttributes` is called after
> > > > > > > parsing an initial sequence of GNU attributes that might be part
> > > > > > > of the decl-specifier-seq, or might precede a label. In this
> > > > > > > case, we should simply have two attribute lists: the C++11-syntax
> > > > > > > declaration attributes, and the GNU decl-specifier-seq attributes
> > > > > > > that we've effectively tentatively parsed past.
> > > > > > >
> > > > > > > All other code paths into `ParseExternalDeclaration` /
> > > > > > > `ParseDeclaration` / `ParseSimpleDeclaration` parse only C++11
> > > > > > > attribute syntax before getting there.
> > > > > > >
> > > > > > > With those two exceptions changed, we should be able to keep the
> > > > > > > declaration attributes entirely separate from the
> > > > > > > decl-specifier-seq attributes. We can then carefully re-implement
> > > > > > > the "sliding" of declaration attributes onto the type in the
> > > > > > > cases where we want to do so for compatibility.
> > > > > > > I think the right approach here is that ParseDeclaration,
> > > > > > > ParseExternalDeclaration, and friends should only be given an
> > > > > > > attribute list that includes declaration attributes (that is,
> > > > > > > C++11 attribute syntax attributes), and that list should not be
> > > > > > > muddled into the list of decl specifier attributes.
> > > > > >
> > > > > > I think this makes sense, but I'm not sure how we would do this in
> > > > > > the specific case of `ParseStatementOrDeclarationAfterAttributes`
> > > > > > -- see discussion below.
> > > > > >
> > > > > > Also, I wanted to make sure we're on the same page about a specific
> > > > > > point: For backwards compatibility, we need to continue to "slide"
> > > > > > some C++11 attributes to the `DeclSpec`, namely those type
> > > > > > attributes for which we have allowed this so far. Do you agree?
> > > > > >
> > > > > > > 1. ParseExportDeclaration parses MS attributes before recursing
> > > > > > > into ParseExternalDeclaration. That looks like it's simply a bug
> > > > > > > as far as I can tell, and that call can be removed. MS attributes
> > > > > > > will be parsed as part of the decl specifier sequence as needed
> > > > > > > and don't need to be parsed as declaration attributes
> > > > > >
> > > > > > That makes sense -- and indeed none of the other callers of
> > > > > > `ParseExternalDeclaration` parse MS attributes before calling it.
> > > > > >
> > > > > > I've tried removing the two calls to
> > > > > > `MaybeParseMicrosoftAttributes` in `ParseExportDeclaration`, and
> > > > > > all of the tests still pass.
> > > > > >
> > > > > > I'm not sure though if this may produce new errors on some invalid
> > > > > > code that we used to allow, namely those cases in
> > > > > > `ParseExternalDeclaration` that call through to `ParseDeclaration`
> > > > > > rather than `ParseDeclOrFunctionDefInternal`. From looking at the
> > > > > > code, one erroneous code sample that I expected the current code to
> > > > > > allow is `export __declspec(deprecated) namespace foo {}`, but it
> > > > > > appears that we don't (https://godbolt.org/z/b4sE8E8GG). So maybe
> > > > > > there's no concern here?
> > > > > >
> > > > > > If we don't think there are any issues with this, I would prepare a
> > > > > > small patch that removes these two calls to
> > > > > > `MaybeParseMicrosoftAttributes`. (I think this can and should be a
> > > > > > separate patch as it's otherwise independent of the wider issue
> > > > > > we're discussing here.)
> > > > > >
> > > > > > > 2. ParseStatementOrDeclarationAfterAttributes is called after
> > > > > > > parsing an initial sequence of GNU attributes that might be part
> > > > > > > of the decl-specifier-seq, or might precede a label. In this
> > > > > > > case, we should simply have two attribute lists: the C++11-syntax
> > > > > > > declaration attributes, and the GNU decl-specifier-seq attributes
> > > > > > > that we've effectively tentatively parsed past.
> > > > > >
> > > > > > To make sure I understand: What you're saying is that
> > > > > > `ParseStatementOrDeclarationAfterAttributes` should take two
> > > > > > `ParsedAttributes` arguments, one for the C++11 attributes and one
> > > > > > for the GNU attributes?
> > > > > >
> > > > > > What I'm not quite clear on is how
> > > > > > `ParseStatementOrDeclarationAfterAttributes` should handle these
> > > > > > two attribute lists further. There are two places in this function
> > > > > > that call through to `ParseDeclaration` and I think you're saying
> > > > > > that you'd like `ParseDeclaration` to only take a list of C++11
> > > > > > attributes. But what should
> > > > > > `ParseStatementOrDeclarationAfterAttributes` then do with the GNU
> > > > > > attributes -- it can't simply drop them on the floor?
> > > > > >
> > > > > > Or does this mean that `ParseDeclaration` also needs to take two
> > > > > > `ParsedAttributes` arguments (and similarly for any other `Parse*`
> > > > > > functions it passes the attributes on to)? This is certainly
> > > > > > doable, but I'm not sure that we get much benefit from it. As noted
> > > > > > at the beginning of this comment, some "legacy" C++11 type
> > > > > > attributes will also need to "slide" to the `DeclSpec` for
> > > > > > backwards compatibility, so we can't simply put all the C++11
> > > > > > attributes on the declaration (which would be the attraction of
> > > > > > having separate lists). At that point, it doesn't seem too harmful
> > > > > > to me to have a single `ParsedAttributes` argument containing all
> > > > > > of the attributes (C++11 and GNU) that were specified at the
> > > > > > beginning of the declaration.
> > > > > >
> > > > > > > With those two exceptions changed, we should be able to keep the
> > > > > > > declaration attributes entirely separate from the
> > > > > > > decl-specifier-seq attributes. We can then carefully re-implement
> > > > > > > the "sliding" of declaration attributes onto the type in the
> > > > > > > cases where we want to do so for compatibility.
> > > > > >
> > > > > > This is essentially what https://reviews.llvm.org/D124919 does. To
> > > > > > emphasize this (and simplify the code slightly at the same time),
> > > > > > I've replaced the previous `ExtractDefiniteDeclAttrs()` with a
> > > > > > function called `SlideAttrsToDeclSpec()` that moves the "sliding"
> > > > > > attributes from the original `ParsedAttributes` directly to the
> > > > > > `DeclSpec`. Is this along the lines of what you were thinking of?
> > > > > > For backwards compatibility, we need to continue to "slide" some
> > > > > > C++11 attributes to the `DeclSpec`, namely those type attributes
> > > > > > for which we have allowed this so far. Do you agree?
> > > > >
> > > > > I think we should allow this in the short term, but aim to remove it
> > > > > after we've been warning on it for a release or two. I'd even be OK
> > > > > with the warning being an error by default when it lands.
> > > > >
> > > > > > From looking at the code, one erroneous code sample that I expected
> > > > > > the current code to allow is `export __declspec(deprecated)
> > > > > > namespace foo {}`, but it appears that we don't
> > > > > > (https://godbolt.org/z/b4sE8E8GG). So maybe there's no concern here?
> > > > >
> > > > > MS attributes are `[`...`]` attributes, so the testcase would be
> > > > > `export [blah] namespace foo {}`, which we do currently erroneously
> > > > > accept. Still, I think that's highly unlikely to be a concern. Our
> > > > > support for C++20 modules is new, experimental, and incomplete;
> > > > > there's no need to worry about backwards-incompatibility here.
> > > > >
> > > > > > To make sure I understand: What you're saying is that
> > > > > > `ParseStatementOrDeclarationAfterAttributes` should take two
> > > > > > `ParsedAttributes` arguments, one for the C++11 attributes and one
> > > > > > for the GNU attributes?
> > > > >
> > > > > I would phrase that as: one for declaration attributes, and one for
> > > > > decl-specifier attributes (put another way: one parameter for
> > > > > declaration attributes, and one parameter for the portion of a
> > > > > decl-specifier-seq that we parsed while looking for a label, which
> > > > > happens to be only GNU attributes). But yes.
> > > > >
> > > > > > What I'm not quite clear on is how
> > > > > > `ParseStatementOrDeclarationAfterAttributes` should handle these
> > > > > > two attribute lists further. There are two places in this function
> > > > > > that call through to `ParseDeclaration` and I think you're saying
> > > > > > that you'd like `ParseDeclaration` to only take a list of C++11
> > > > > > attributes. But what should
> > > > > > `ParseStatementOrDeclarationAfterAttributes` then do with the GNU
> > > > > > attributes -- it can't simply drop them on the floor?
> > > > > >
> > > > > > Or does this mean that `ParseDeclaration` also needs to take two
> > > > > > `ParsedAttributes` arguments (and similarly for any other `Parse*`
> > > > > > functions it passes the attributes on to)? This is certainly
> > > > > > doable, but I'm not sure that we get much benefit from it. As noted
> > > > > > at the beginning of this comment, some "legacy" C++11 type
> > > > > > attributes will also need to "slide" to the `DeclSpec` for
> > > > > > backwards compatibility, so we can't simply put all the C++11
> > > > > > attributes on the declaration (which would be the attraction of
> > > > > > having separate lists).
> > > > >
> > > > > Yes, we should propagate the two lists into `ParseDeclaration` and
> > > > > `ParseSimpleDeclaration`. That should be the end of it:
> > > > > `ParseSimpleDeclaration` forms a `ParsingDeclSpec`, which can be
> > > > > handed the decl-specifier attributes. The other cases that
> > > > > `ParseDeclaration` can reach should all prohibit decl-specifier
> > > > > attributes.
> > > > >
> > > > > > At that point, it doesn't seem too harmful to me to have a single
> > > > > > `ParsedAttributes` argument containing all of the attributes (C++11
> > > > > > and GNU) that were specified at the beginning of the declaration.
> > > > >
> > > > > I think it will be best for ongoing maintenance if we model the
> > > > > grammar in the parser as it is -- C++11 declaration attributes are a
> > > > > separate list that's not part of the decl-specifier-seq, and I expect
> > > > > we will have emergent bugs and confusion if we try to combine them.
> > > > > Attribute handling in declaration parsing is already made confusing
> > > > > because we don't clearly distinguish between these two lists, leading
> > > > > to bugs like parsing MS attributes in `ParseExportDeclaration`.
> > > > >
> > > > > > > With those two exceptions changed, we should be able to keep the
> > > > > > > declaration attributes entirely separate from the
> > > > > > > decl-specifier-seq attributes. We can then carefully re-implement
> > > > > > > the "sliding" of declaration attributes onto the type in the
> > > > > > > cases where we want to do so for compatibility.
> > > > > >
> > > > > > This is essentially what https://reviews.llvm.org/D124919 does. To
> > > > > > emphasize this (and simplify the code slightly at the same time),
> > > > > > I've replaced the previous `ExtractDefiniteDeclAttrs()` with a
> > > > > > function called `SlideAttrsToDeclSpec()` that moves the "sliding"
> > > > > > attributes from the original `ParsedAttributes` directly to the
> > > > > > `DeclSpec`. Is this along the lines of what you were thinking of?
> > > > >
> > > > > Approximately. I also think we should avoid moving attributes around
> > > > > between attribute lists entirely, and instead preserve the distinct
> > > > > attribute lists until we reach the point where we can apply them to a
> > > > > declaration in `Sema`. I would note that the approach in that patch
> > > > > seems like it'll reorder attributes, and we have attributes where the
> > > > > order matters (eg, `[[clang::enable_if(a, "")]] void f
> > > > > [[clang::enable_if(b, "")]]()`). Also, moving attributes around makes
> > > > > it hard to properly handle source locations / ranges for attributes.
> > > > >
> > > > > In particular: I would make the `ParsingDeclarator` have a pointer to
> > > > > the declaration attributes in addition to its list of attributes on
> > > > > the declarator-id. Then, when we apply those attributes to the
> > > > > declaration, pull them from both places (declaration attributes then
> > > > > declarator-id attributes, to preserve source order, skipping "slided"
> > > > > type attributes in the former list). And when we form the type for
> > > > > the declarator, also apply any attributes from the declaration's
> > > > > attribute list that should be "slided" on the type.
> > > > (Oops -- only just realized that I wrote up this comment in draft form
> > > > last week but never sent it.)
> > > >
> > > > Thank you for the really in-depth reply!
> > > >
> > > > > > For backwards compatibility, we need to continue to "slide" some
> > > > > > C++11 attributes to the `DeclSpec`, namely those type attributes
> > > > > > for which we have allowed this so far. Do you agree?
> > > > >
> > > > > I think we should allow this in the short term, but aim to remove it
> > > > > after we've been warning on it for a release or two. I'd even be OK
> > > > > with the warning being an error by default when it lands.
> > > >
> > > > And there would be a command-line flag that would allow "downgrading"
> > > > the error to a warning?
> > > >
> > > > > > From looking at the code, one erroneous code sample that I expected
> > > > > > the current code to allow is `export __declspec(deprecated)
> > > > > > namespace foo {}`, but it appears that we don't
> > > > > > (https://godbolt.org/z/b4sE8E8GG). So maybe there's no concern here?
> > > > >
> > > > > MS attributes are `[`...`]` attributes,
> > > >
> > > > Oops *facepalm* A closer look at `ParseMicrosoftAttributes` could have
> > > > told me that. I didn't even know of these until now TBH.
> > > >
> > > > > so the testcase would be `export [blah] namespace foo {}`, which we
> > > > > do currently erroneously accept. Still, I think that's highly
> > > > > unlikely to be a concern. Our support for C++20 modules is new,
> > > > > experimental, and incomplete; there's no need to worry about
> > > > > backwards-incompatibility here.
> > > >
> > > > Got it. I'll submit a separate small patch that removes those two calls
> > > > to `MaybeParseMicrosoftAttributes` then.
> > > >
> > > > > > To make sure I understand: What you're saying is that
> > > > > > `ParseStatementOrDeclarationAfterAttributes` should take two
> > > > > > `ParsedAttributes` arguments, one for the C++11 attributes and one
> > > > > > for the GNU attributes?
> > > > >
> > > > > I would phrase that as: one for declaration attributes, and one for
> > > > > decl-specifier attributes (put another way: one parameter for
> > > > > declaration attributes, and one parameter for the portion of a
> > > > > decl-specifier-seq that we parsed while looking for a label, which
> > > > > happens to be only GNU attributes). But yes.
> > > >
> > > > Got it, that makes sense.
> > > >
> > > > [snip]
> > > >
> > > > > > This is essentially what https://reviews.llvm.org/D124919 does. To
> > > > > > emphasize this (and simplify the code slightly at the same time),
> > > > > > I've replaced the previous `ExtractDefiniteDeclAttrs()` with a
> > > > > > function called `SlideAttrsToDeclSpec()` that moves the "sliding"
> > > > > > attributes from the original `ParsedAttributes` directly to the
> > > > > > `DeclSpec`. Is this along the lines of what you were thinking of?
> > > > >
> > > > > Approximately. I also think we should avoid moving attributes around
> > > > > between attribute lists entirely, and instead preserve the distinct
> > > > > attribute lists until we reach the point where we can apply them to a
> > > > > declaration in `Sema`. I would note that the approach in that patch
> > > > > seems like it'll reorder attributes, and we have attributes where the
> > > > > order matters (eg, `[[clang::enable_if(a, "")]] void f
> > > > > [[clang::enable_if(b, "")]]()`). Also, moving attributes around makes
> > > > > it hard to properly handle source locations / ranges for attributes.
> > > >
> > > > Thanks for pointing out the importance of ordering. I did notice some
> > > > ordering issues while working on https://reviews.llvm.org/D124919 (see
> > > > comments in SlideAttrsToDeclSpec() if you're interested), but this
> > > > drives home the point that we need to be careful about ordering.
> > > >
> > > > > In particular: I would make the `ParsingDeclarator` have a pointer to
> > > > > the declaration attributes in addition to its list of attributes on
> > > > > the declarator-id. Then, when we apply those attributes to the
> > > > > declaration, pull them from both places (declaration attributes then
> > > > > declarator-id attributes, to preserve source order, skipping "slided"
> > > > > type attributes in the former list). And when we form the type for
> > > > > the declarator, also apply any attributes from the declaration's
> > > > > attribute list that should be "slided" on the type.
> > > >
> > > > Thanks, I think I can see how that's going to work.
> > > >
> > > > I'll attempt to implement this, which will likely take a few days, and
> > > > will report back when I have something that works and that I'd like to
> > > > get feedback on. I may also have some followup questions if I run into
> > > > unanticipated problems (which is more likely than not, I assume).
> > > @rsmith I have a draft implementation of the idea we discussed above here:
> > >
> > > https://reviews.llvm.org/D126061
> > >
> > > (All tests pass.)
> > >
> > > I'd appreciate your feedback. Does this look like what you had in mind?
> > >
> > > There's one main point I'd like to discuss. I've chosen to store the
> > > declaration attributes in a new field on the `Declarator`, so we now have
> > > `Declarator::Attrs` and `Declarator::DeclarationAttrs` (and I believe
> > > this is what we had discussed before). This makes sense from a semantic
> > > point of view because attributes in both positions have the same meaning.
> > >
> > > However there's one point I'm not completely satisfied with: Because a
> > > single declaration can have multiple declarators, we need to make sure
> > > that we preserve the declaration attributes when we reuse a `Declarator`
> > > for multiple declarators in a single declaration. This gives rise to the
> > > slightly unfortunate `Declarator::clearExceptDeclarationAttrs()`.
> > >
> > > All in all, I feel the implementation would be cleaner if we put the
> > > declaration attributes on the `DeclSpec` instead of the `Declarator`,
> > > even if the declaration attributes are semantically less related to the
> > > `DeclSpec` than the `Declarator`. What do you think?
> > PS Another option would be to introduce a `Declaration` class and put the
> > declaration attributes on that. The `Declarator` would hold a reference to
> > the `Declaration`.
> >
> > This solution would represent the actual entities in the grammar best, but
> > I wonder if it's worth introducing a `Declarator` class for this purpose
> > alone. There might also be potential for confusion because we already have
> > a `Decl` class in the AST.
> Some notes for those following along:
>
> @rsmith and @aaron.ballman are both in the process of reviewing
> https://reviews.llvm.org/D126061 and have stated that they think this is the
> right direction.
>
> I also wanted to clarify how I want to proceed with this change and
> https://reviews.llvm.org/D126061. I believe I originally said that I wanted
> to land https://reviews.llvm.org/D126061 first, then this change afterwards.
>
> However, I've now changed my mind about this. We want `annotate_type` to be
> available in https://reviews.llvm.org/D126061 because `annotate_type` is the
> first type attribute that doesn't exhibit the legacy "sliding" behavior.
> Without it, https://reviews.llvm.org/D126061 can't introduce some of the new
> behavior it introduces.
>
> So my plan is now to wait until both this change and
> https://reviews.llvm.org/D126061 are ready to land, and then land them in
> sequence (this change first, then https://reviews.llvm.org/D126061).
> So my plan is now to wait until both this change and
> https://reviews.llvm.org/D126061 are ready to land, and then land them in
> sequence (this change first, then https://reviews.llvm.org/D126061).
That plan is fine by me.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111548/new/
https://reviews.llvm.org/D111548
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
