[PATCH] D68315: [libTooling] Add various Stencil combinators for expressions.

[Dmitri Gribenko via Phabricator via cfe-commits]

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