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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
