aaron.ballman added a comment.
In D86559#2369317 <https://reviews.llvm.org/D86559#2369317>, @Mordante wrote:
> Then me try to clear up the confusion.
> The parser first parses the `LabelDecl` and tries to attach the attributes to
> this declaration. If that fails instead of issue a diagnostic for a
> `StmtAttr` attached to a declaration it let's them be attached to the
> `LabelStmt`. This way attributes can be placed on both the `LabelDecl` and
> the `LabelStmt`. For example:
>
> void foo() {
> __label__ lbl; // Needed to show the LabelDecl in the AST
> [[likely, clang::annotate("foo")]] lbl:;
> }
>
> Will result in the following AST
>
> `-FunctionDecl 0x560be00b6728 </tmp/x.cpp:1:1, line:4:1> line:1:6 foo 'void
> ()'
> `-CompoundStmt 0x560be00b6a00 <col:12, line:4:1>
> |-DeclStmt 0x560be00b6860 <line:2:3, col:16>
> | `-LabelDecl 0x560be00b6810 <col:3, col:13> col:13 lbl
> | `-AnnotateAttr 0x560be00b6920 <line:3:13, col:34> "foo"
> | `-StringLiteral 0x560be00b68f8 <col:29> 'const char [4]' lvalue
> "foo"
> `-AttributedStmt 0x560be00b69e8 <col:3, col:42>
> |-LikelyAttr 0x560be00b69c0 <col:5>
> `-LabelStmt 0x560be00b69a8 <col:38, col:42> 'lbl'
> `-NullStmt 0x560be00b6918 <col:42>
>
> I thought this was a sensible approach where I don't change the behaviour of
> `DeclAttr` on the `LabelDecl`, but at the same time allow the `StmtAttr` to
> be "forwarded" to the `LabelStmt`, turning it into an `AttributedStmt` with
> the `LabelStmt` as substatement.
I think this is a defensible approach, FWIW.
>> However, I could imagine there being cases when we might want a helper
>> function on `LabelDecl` that looks for attributes on the associated
>> `LabelStmt` and returns results about it if that helps ease implementation
>> or refactoring burdens.
>
> If we want that we need to change the `LabelDecl` to point to either a
> `LabelStmt` or an `AttributedStmt`. This was the approach I thought I had to
> take, but I found this solution. We can still take that direction if wanted.
> But then we need to place `DeclAttr` in an `AttributedStmt`, not sure how
> well that works and how much additional code needs to change to find the
> attributes there. Since in that case we to call this helper function at the
> appropriate places.
Ah, I was thinking of something slightly different here. I was thinking that
`LabelDecl` would hold a `Stmt*` so that it could be either a label statement
or an attribute statement. The helper functions would give you access to the
attributes of the statement and to the `LabelStmt` itself (stripping off any
attributed statements). Then in Attr.td, we'd move attributes off the label
declarations and onto the label statements. At the end of the day, all
attributes on labels would appertain to the statement at the AST level, but
you'd have an easier time transitioning in some places if you could get to the
attributes if the only thing you have is the `LabelDecl`. (So this doesn't mix
statement and declaration attributes together, it shifts the model to putting
all attributes on labels on the statement level rather than having a somewhat
odd split between decl and statement.)
> Does this clear your confusion?
> Do you agree with this approach or do you think changing the `LabelDecl` is
> the better solution?
Thank you for the explanations, I understand your design better now. I'm not
certain what the right answer is yet, but thinking out loud about my concerns:
I worry that making a distinction between a label statement and a label
declaration (at least for attributes) generates heat without light. Making the
attribute author decide "which label construct should my attribute appertain
to" adds extra burden on attribute authors and I'm not sure I have any advice
for them on whether to use the decl or the statement -- it seems entirely
arbitrary. Coupled with the fact that the standard defines labels as being
statements, that suggests to me that putting all of the attributes on the
statement level is the right decision -- it's one place (removes confusion
about where it goes), it's the "obvious" place (matches where the standard
suggests that attributes live), and we should have all the machinery in place
to make it possible within the compiler (given that you can reach the
`LabelStmt` from the `LabelDecl`).
Any thoughts from @rjmccall or @rsmith?
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
