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