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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Given that some parts of this confused me, I'd like to make sure I'm clear on the approach you're taking. Based on the fact that labels are statements (defined as [stmt.label], in the statment production, etc), I would expect that attributes which appertain to labels all appertain to the statement and not the declaration and that the underlying issue is that we attach attributes to the label declaration (such as for `__attribute__((unused))`. So I wasn't expecting that we'd slide attributes around, but redefine which construct they live on (they'd always make an `AttributedStmt` for the label rather than adding the attribute to the `Decl`). 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. WDYT? Also, does @rsmith have opinions here?



================
Comment at: clang/include/clang/Sema/Sema.h:4528
+  StmtResult ActOnLabelStmt(SourceLocation IdentLoc,
+                            const ParsedAttributesView *Attrs,
+                            SourceRange AttrsRange, LabelDecl *TheDecl,
----------------
Don't we have a `ParsedAttributesViewWithRange` (or something along those lines) that could be used instead of separating attributes and their source range?


================
Comment at: clang/lib/Parse/ParseStmt.cpp:677
+  }
+  SourceRange AttributeRange = attrs.Range;
   attrs.clear();
----------------
I think I would have expected this to be calling `Actions.ProcessStmtAttributes()` as done a few lines up on 651 rather than changing the interface to `ActOnLabelStmt()`.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7368
+    // The statement attributes attached to a LabelDecl are handled separately.
+    if (!isa<LabelDecl>(D))
+      S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)
----------------
This also surprises me -- I wouldn't have expected the attribute to be in the list for the label decl in the first place.


================
Comment at: clang/lib/Sema/TreeTransform.h:1307
+    // already properly rebuild. Only the LabelStmt itself needs to be rebuild.
+    return SemaRef.ActOnLabelStmt(IdentLoc, nullptr, SourceLocation(), L,
+                                  ColonLoc, SubStmt);
----------------
I wouldn't have expected any changes to be needed here because the attributed statement should be rebuilt properly by `RebuildAttributedStmt()`, no?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86559/new/

https://reviews.llvm.org/D86559