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

Wed Oct 2 15:05:38 PDT 2019

ymandel marked an inline comment as done.
ymandel 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);

----------------
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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68315/new/

https://reviews.llvm.org/D68315