[PATCH] D86559: [Sema, CodeGen] Allow [[likely]] and [[unlikely]] on labels

[Aaron Ballman via Phabricator via cfe-commits]

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