ymandel marked an inline comment as done. ymandel added inline comments.
================ Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:87 + TextGenerator Replacement; + TextGenerator Explanation; +}; ---------------- ilya-biryukov wrote: > ymandel wrote: > > ilya-biryukov wrote: > > > ymandel wrote: > > > > ilya-biryukov wrote: > > > > > ymandel wrote: > > > > > > ilya-biryukov wrote: > > > > > > > I would've expected explanation to be the trait of the rewrite > > > > > > > rule, since all changes have to be applied. > > > > > > > What's the reasoning behind having it at the level of separate > > > > > > > changes? How would this explanation be used? For debugging > > > > > > > purposes or displaying that to the user? > > > > > > I think that for most cases, one explanation sufficies for the > > > > > > whole transformation. However, there are some tidies that emit > > > > > > multiple diagnoses (for example if changing before a declaration > > > > > > and a definition). Would it help if I clarify in the comments? > > > > > Yeah, absolutely! Please document what it's used for and that would > > > > > clear that up for me. > > > > > I actually thing that explaining every part of the transformation is > > > > > probably too complicated, so most of the time you would want to have > > > > > an explanation for the `RewriteRule`, not for each individual change. > > > > > > > > > > The other challenge that I see is show to display these explanations > > > > > to the user, i.e. how should the clients combine these explanations > > > > > in order to get the full one? Should the `RewriteRule` have an > > > > > explanation of the full transformation too? > > > > I've revised the comments, changed the name to "Note" and put > > > > "Explanation" back into RewriteRule. > > > > > > > > As for how to display these, I imagine an interface like clang tidy's > > > > fixit hints. The Explanation (if any) will be associated with the span > > > > of the entire match. The Notes will be associated with the target node > > > > span of each annotated change. WDYT? > > > Do we really need the AST information to expose the edits to the users? > > > IIUC, clang-tidy uses the information from textual replacements to render > > > the changes produced by the fix-its today. > > > > > > I guess it might be useful to add extra notes to clang-tidy warnings that > > > have corresponding fix-its, but is the transformers library the right > > > layer to produce those? > > > I haven't seen the proposed glue to clang-tidy yet, maybe that would make > > > more sense when I see it. > > > > > > One of the other reasons I ask this is that it seems that without `Note` > > > we don't strictly `ASTEditBuilder`, we could replace > > > ``` > > > change("ref").to("something"); // without nodepart > > > change("ref", NodePart::Member).to("something"); > > > ``` > > > with > > > ``` > > > change("ref", "something") > > > change("ref", NodePart::Member, "something"); > > > ``` > > > That would remove the boilerplate of the builder, simplifying the code a > > > bit. > > > > > > That trick would be hard to pull if we have a `Note` field inside, we'll > > > need more overloads and having note and replacement after each other > > > might be confusing (they have the same type, might be hard to read > > > without the guiding `.to()` and `.because()`) > > Breaking this explicitly into two questions: > > 1. Do Notes belong here? > > 2. Can we drop the builder? > > > > 1. Do Notes belong here? > > I think so. When users provide a diagnostic, they are specifying its > > location. So, we don't quite need the AST but we do need location info. > > The diagnostic generator does take information from the replacements > > themselves, but that's not alone. For example: > > clang-tidy/readability/ConstReturnTypeCheck.cpp:104. That demos both the > > diagnostic construction and multiple diagnostics in a single tidy result. > > > > Given that, i think that RewriteRules are the appropriate place. The goal > > is that they be self contained, so I think the explanations should be > > bundled with the description of that actual change. An example approach to > > merging with clang tidy is here: > > https://github.com/ymand/llvm-project/blob/transformer/clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp > > (although that's based on a somewhat different version of the Transformer > > library; it should make clear what I have in mind). > > > > 2. Do we need the builder? > > No, despite my opinion that notes belong. Given the explanation on > > RewriteRule, I think notes will be used only rarely (like in the example > > clang-tidy above; but that's not standard practice). So, we can provide > > the two overloads of `change` that you mention and then let users assign > > the note directly in those rare cases they need it. then, we can drop the > > builder. > > > > So, I propose keeping the note but dropping the builder. WDYT? > Thanks for pointing out how this usage in clang-tidy. Seems to be the only > way to put it into clang-tidy (and there are good reasons for that, e.g. > having the full context of the change). > > > So, I propose keeping the note but dropping the builder. WDYT? > LGTM! > > Removed the builder, added more `change` overloads. Also renamed `changeRoot` to `changeMatched` because I think it's a better description. The only part I don't love (here and elsewhere) is the duplicate overloads one each for `std::string` and `TextGenerator`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60408/new/ https://reviews.llvm.org/D60408 _______________________________________________ cfe-commits mailing list email@example.com https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits