aaron.ballman added a comment. In D86559#2371581 <https://reviews.llvm.org/D86559#2371581>, @Mordante wrote:
> In D86559#2371058 <https://reviews.llvm.org/D86559#2371058>, @aaron.ballman > wrote: > >> In D86559#2369317 <https://reviews.llvm.org/D86559#2369317>, @Mordante wrote: >> >>> Then me try to clear up the confusion. >>> >>>> 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. > > Yes I wanted to take that route. While investigating that route I found the > current solution and it seemed less intrusive. So that's why I went for the > current approach. > >> 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.) > > If we go that route, then we need to think about attributes that are normally > allowed on declarations. For example > > def Unused : InheritableAttr { > let Spellings = [CXX11<"", "maybe_unused", 201603>, GCC<"unused">, > C2x<"", "maybe_unused", 201904>]; > let Subjects = SubjectList<[Var, ObjCIvar, Type, Enum, EnumConstant, > Label, > Field, ObjCMethod, FunctionLike]>; > let Documentation = [WarnMaybeUnusedDocs]; > } Yup, `annotate` is another such attribute, to some degree. I was envisioning that these tablegen definitions would have to change slightly to allow for writing on a statement for the label case. > It also seems attributes on the `LabelDecl` aren't visible in the AST at the > moment. If I modify the example posted yesterday to: > > void foo() { > [[likely, clang::annotate("foo")]] lbl:; > } > > The AST will become: > > `-FunctionDecl 0x5607f7dbb7a8 </tmp/x.cpp:1:1, line:3:1> line:1:6 foo 'void > ()' > `-CompoundStmt 0x5607f7dbba60 <col:12, line:3:1> > `-AttributedStmt 0x5607f7dbba48 <line:2:3, col:42> > |-LikelyAttr 0x5607f7dbba20 <col:5> > `-LabelStmt 0x5607f7dbba08 <col:38, col:42> 'lbl' > `-NullStmt 0x5607f7dbb928 <col:42> > > Maybe it would be a good idea to see whether the `LabelDecl` needs to be > visible in the AST even if we proceed with the current approach. Good catch, I hadn't noticed that before. I agree, we should figure that out. Some simple testing shows clang-query also doesn't match label declarations with the `decl()` matcher, so the issue is wider than just dumping the AST. >>> 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`). > > I agree it's not clear cut what the best way is. For example `unused` is a > declaration attribute, but `likely` is a statement attribute. Both can be > used on a label. `unused` is also a bit weird because `[[maybe_unused]]` technically does not apply to labels (http://eel.is/c++draft/dcl.attr.unused#2, we are using a conforming extension in our implementation), but `__attribute__((unused))` does apply to labels. > I think for authors in most cases it will be clear, their attribute is also > used on other declarations or statements. The harder case will be, when a > label specific attribute is added, should it then go on the declaration or > the statement. I foresee another issue when there will be attributes that can > be attached to both a declaration and a statement, for example if we > implement `[[clang::suppress("compiler.warning.12345")]]`. (But I suspect > that attribute has more implementation challenges.) In the current approach > that attribute will be attached to the declaration since that's parsed first. > I think regardless of the final solution labels will remain a special case > for attributes. I'm more and more thinking of `LabelDecl` as an implementation detail that allows us to perform lookups easier rather than a salient piece of the AST, so I guess I view label-specific attributes as being the easy case where the attributes consistently go on the statement level. > During the review of `[[clang::nomerge]]` the placement on labels was also > discussed. I also see some threads on the dev-ml about making more use of > attributes. For example to enhance the analyzers, which sounds very > interesting. So I feel it's worth the effort to look for the "best" approach, > even if the usefulness for the likelihood attributes is limited. I agree and I greatly appreciate your patience while we work the design out. 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
