Mordante planned changes to this revision.
Mordante added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6454
+static bool validateLikelihoodAttr(Sema &S, Decl *D, const ParsedAttr &A) {
+ if (!isa<LabelDecl>(D)) {
----------------
aaron.ballman wrote:
> Mordante wrote:
> > aaron.ballman wrote:
> > > This is entering into somewhat novel territory for attributes, so some of
> > > this feedback is me thinking out loud.
> > >
> > > Attr.td lists these two attributes as being a `StmtAttr` and not any kind
> > > of declaration attribute. We have `DeclOrTypeAttr` for attributes that
> > > can be applied to declarations or types, but we do not have something
> > > similar for statement attributes yet. We do have some custom semantic
> > > handling logic in SemaDeclAttr.cpp for statement attributes, but you
> > > avoid hitting that code path by adding a `case` for the two likelihood
> > > attributes. These attributes only apply to label declarations currently,
> > > and labels cannot be redeclared, so there aren't yet questions about
> > > whether this is inheritable or not. So we *might* be okay with this, but
> > > I'm not 100% certain. For instance, I don't recall if the pretty printer
> > > or AST dumpers will need to distinguish between whether this attribute is
> > > written on the statement or the declaration (which is itself a bit of an
> > > interesting question: should the attribute attach only to the statement
> > > rather than trying to attach to the underlying decl?
> > > http://eel.is/c++draft/stmt.stmt#stmt.label-1.sentence-2 is ambiguous,
> > > but I don't think of `case` or `default` labels as being declarations so
> > > I tend to not think of identifier labels as such either.). There's a part
> > > of me that wonders if we have a different issue where the attribute is
> > > trying to attach to the declaration rather than the statement and that
> > > this should be handled purely as a statement attribute.
> > >
> > > I'm curious what your thoughts are, but I'd also like to see some
> > > additional tests for the random other bits that interact with attributes
> > > like AST dumping and pretty printing to be sure the behavior is
> > > reasonable.
> > The labels in a switch are indeed different and the code in trunk already
> > should allow the attribute there. (I'm still busy with the CodeGen patch.)
> > I agree that Standard isn't clear whether the attribute is attached to the
> > statement or the declaration.
> >
> > The `LabelDecl` expects a pointer to a `LabelStmt` and not to an
> > `AttributedStmt`. Since declarations can already have attributes I used
> > that feature. I just checked and the `LabelDecl` isn't shown in the AST and
> > so the attributes also aren't shown. I can adjust that.
> >
> > Another option would be to change the `LabelDecl` and have two overloads of
> > `setStmt`
> > `void setStmt(LabelStmt *T) { TheStmt = T; }`
> > `void setStmt(AttributedStmt *T) { TheStmt = T; }`
> > Then `TheStmt` needs to be a `Stmt` and an extra getter would be required
> > to get the generic statement.
> >
> > I think both solutions aren't trivial changes. Currently the attribute has
> > no effect on labels so it not being visible isn't a real issue. However I
> > feel that's not a proper solution. I expect attributes will be used more in
> > C and C++ in the future. For example, I can imagine a `[[cold]]` attribute
> > becoming available for labels.
> >
> > So I'm leaning towards biting the bullet and change the implementation of
> > `LabelDecl` to allow an `AttributedStmt` instead of a `LabelStmt`.
> > WDYT?
> > Currently the attribute has no effect on labels so it not being visible
> > isn't a real issue.
>
> That's not entirely true though -- we have pretty printing capabilities that
> will lose the attribute when written on a label, so round-tripping through
> the pretty printer will fail. But we have quite a few issues with pretty
> printing attributes as it stands, so I'm not super concerned either.
>
> > So I'm leaning towards biting the bullet and change the implementation of
> > LabelDecl to allow an AttributedStmt instead of a LabelStmt.
> > WDYT?
>
> I'm curious if @rsmith feels the same way, but I think something along those
> lines makes sense (if not an overload, perhaps a templated function with
> SFINAE). We'd have to make sure that the attributed statement eventually
> resolves to a `LabelStmt` once we strip the attributes away, but this would
> keep the attribute at the statement level rather than making it a declaration
> one, which I think is more along the lines of what's intended for the
> likelihood attributes (and probably for hot/cold if we add that support
> later).
> > Currently the attribute has no effect on labels so it not being visible
> > isn't a real issue.
>
> That's not entirely true though -- we have pretty printing capabilities that
> will lose the attribute when written on a label, so round-tripping through
> the pretty printer will fail. But we have quite a few issues with pretty
> printing attributes as it stands, so I'm not super concerned either.
I'll keep that in mind when I start working on that.
>
> > So I'm leaning towards biting the bullet and change the implementation of
> > LabelDecl to allow an AttributedStmt instead of a LabelStmt.
> > WDYT?
>
> I'm curious if @rsmith feels the same way, but I think something along those
> lines makes sense (if not an overload, perhaps a templated function with
> SFINAE). We'd have to make sure that the attributed statement eventually
> resolves to a `LabelStmt` once we strip the attributes away, but this would
> keep the attribute at the statement level rather than making it a declaration
> one, which I think is more along the lines of what's intended for the
> likelihood attributes (and probably for hot/cold if we add that support
> later).
Yes if we go for an overload I need to make sure that the attributed statement
has a `LabelStmt` as its substatement. I haven't looked into how to enforce
that.
@rsmith Any opinion on whether the likelihood attribute should be "attached" to
the label declaration or the label statement?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86559/new/
https://reviews.llvm.org/D86559
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
