gribozavr marked an inline comment as done. gribozavr added inline comments.
================ Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:149 /// \returns the source corresponding to the selected range. StencilPart selection(RangeSelector Selector); ---------------- ymandel wrote: > gribozavr wrote: > > ymandel wrote: > > > gribozavr wrote: > > > > Should the comment cross-reference expression() and say that the user > > > > probably wants that instead? > > > That depends on what selector they're using. For > > > `selection(node(ExprId))`, yes I think that `expression(ExprId)` is going > > > to be better in most cases. But, for other selectors, no. So, I'm not > > > sure that the cross-reference will be generally useful. WDYT? > > > > > > Also, it occurs to me that we have an asymmetry for statements and > > > expressions. Getting the source of a statement is > > > `selection(statement(Id))` versus `expression(Id)` for expressions. > > > However, in the context of `cat`, which takes `RangeSelector` directly, > > > they look the same, because `selection` isn't needed. > > Yeah, you're right -- there are different cases, and `selection()` is not > > just for expressions. Maybe just point out that digging out raw source code > > should be considered a fallback option, and other AST-aware and > > context-aware stencils should be preferred where they exist. > I'm still not sure what advice we want to give. In general, selection() is > the way to go any time you want to propagate code from source to the target, > particularly for cases where there is no corresponding node. So, in many > cases it will be the only (and correct) option, rather than a fallback. > > So, I'm inclined to leave it as is. The next major task I'm working on for > Transformer is to write documentation for the set of related langauges here > (that is, user-manual style documentation). I think that effort should help > clarify what we want in some of these comments. WDYT? Okay, probably I don't understand it sufficiently then. Feel free to leave it as is, especially since this comment was there before this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68315/new/ https://reviews.llvm.org/D68315 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
