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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Sep 27 06:03:00 PDT 2020


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)) {
----------------
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.


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

https://reviews.llvm.org/D86559



More information about the cfe-commits mailing list