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