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:
> 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?
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
