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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 8 inline comments as done.
ymandel added inline comments.


================
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)...);
----------------
gribozavr2 wrote:
> 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.
> 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?

Actually, that was the original design of the library.  We changed it in the first submitted version on Ilya's suggestion to avoid the dependency between Transformer and Stencil.  In fact, the reason TextGenerator exists is to abstract away that dependency.

Looking back, I think it was a reasonable choice at the time, keeping the commits, etc. independent. 
But, based on my experience with the library over the last six months, I'd say these are clearly a single package and we'd be better off re-introducing the dependency in exactly the way you describe.

I think we can start (here) by taking the Stencil as an argument and wrapping it as a TextGenerator. But, I should follow up with a cleanup to actually store the value as a Stencil rather than wrapped as a TextGenerator.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69613