[PATCH] D69613: [libTooling] Simplify type structure of `Stencil`s.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:82
+Stencil makeStencil(RangeSelector Selector);
+inline Stencil makeStencil(Stencil S) { return S; }
 
----------------
ymandel wrote:
> gribozavr2 wrote:
> > I feel like this should be an implementation detail of `cat` (or other stencil combinators who want to support such implicit conversions). Users writing stencils should use concrete factory functions below.
> > 
> > 
> Agreed, but I wasn't sure if it was worth introducing a "detail" namespace for just one (overloaded) function. I liked when these were private methods of Stencil, but we don't have that option now. Should I use a `namespace detail`? something else? 
I don't have a strong opinion, but `namespace detail` should be fine.


================
Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:137
+/// number.
+Stencil sequence(std::vector<Stencil> Parts);
 
----------------
ymandel wrote:
> gribozavr2 wrote:
> > It is the same operation as `cat`, right? So WDYT about `cat_vector`?
> not quite. `cat` is "smart" and optimizes case of one argument to just return that argument. in the future, it might also handle the case of 0 arguments specially. `sequence` just blindly constructs a sequence stencil.
> 
> that said, i can see the argument that the user doesn't care about such details and instead of the three functions:
> ```
> cat(...)
> cat(vector)
> sequence(vector)
> ```
> 
> we should just have the two
> ```
> cat(...)
> catVector(vector)
> ```
> with no "plain" sequence constructor. WDYT?
> i can see the argument that the user doesn't care about such details

I agree, and I would also say that we probably don't even want the users of the library to depend on such details either. So I agree, `cat(...)` and `catVector(vector)` look like the best option to me.


================
Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:154
+/// for more details.
+template <typename... Ts> MatchConsumer<std::string> stencil(Ts &&... Parts) {
+  Stencil S = cat(std::forward<Ts>(Parts)...);
----------------
ymandel wrote:
> gribozavr2 wrote:
> > I'm a bit confused by the name. The function returns a MatchConsumer, but it is called "stencil". What is the intended usage?
> Intended use is something like `makeRule(matcher, change(stencil(...)))`.
> 
> The naming is the same as we previously had for `text()`. Since `MatchConsumer` is so general, the function name describes its argument, not its particular type.  But, I'm open to other suggestions.
I see -- thanks. `change()` takes a `TextGenerator`. Aside from stencils, what are the other text generators? If those are not important, could we change `change()` to take a stencil directly? Or if those other text generators are important, we could add overloads to `change()` that accept a stencil.

My thinking here is that the user is not really concerned with the fact that Stencil-to-TextGenerator conversion is happening here. I think the more complicated operation is the combining of stencils -- and we already have `cat` for that; so adding another way to spell that is better avoided. So, not emphasizing the conversion by making it a part of the `change()` seems fine, and spelling the operation as `cat` seems to be an improvement to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69613