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)) {
----------------
Mordante wrote:
> 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?
> So I'm leaning towards biting the bullet and change the implementation of
> `LabelDecl` to allow an `AttributedStmt` instead of a `LabelStmt`.
Actually it seems this isn't required, it works when `ActOnLabelStmt` returns
an `AttributedStmt`. This means the `LabelDecl` class doesn't need to change.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86559/new/
https://reviews.llvm.org/D86559
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
