Mordante marked 2 inline comments as done. Mordante added a comment. 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]; } 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. >> 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. 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. 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. 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
