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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 5 inline comments as done.
ymandel added a comment.

Thanks for the review! I agreed w/ all the comments, just responding early w/ those I had further thoughts/questions on.



================
Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:82
+Stencil makeStencil(RangeSelector Selector);
+inline Stencil makeStencil(Stencil S) { return S; }
 
----------------
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? 


================
Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:137
+/// number.
+Stencil sequence(std::vector<Stencil> Parts);
 
----------------
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?


================
Comment at: clang/include/clang/Tooling/Transformer/Stencil.h:150
 }
-} // namespace stencil
-} // namespace tooling
+Stencil cat(std::vector<Stencil> Parts);
+
----------------
gribozavr2 wrote:
> Like I said in the other review, I don't feel great about an overload set with universal references and anything else.
Sorry, I'd forgotten we'd discussed this before. I see your point -- I was surprised it worked, but when it did just barelled along. That should have been a red flag. :)  I'm happy to rework this, per the comment above,.


================
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:
> 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.


================
Comment at: clang/lib/Tooling/Transformer/Stencil.cpp:262
+
+template <typename T> class StencilImpl : public StencilInterface {
   T Data;
----------------
gribozavr2 wrote:
> There isn't much logic left in StencilImpl.
> 
> Would the code be simpler if we made the various Data classes implement StencilInterface directly?
Yes! I plan to do that in a followup revision, though I was really tempted to do it here already. :)

That said, we could also use a `llvm::PointerSumType` and implement it in functional style, fwiw. I'm fine either way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69613