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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
