aaron.ballman 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:
> > 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).
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
