[PATCH] D67969: [libTooling] Add `run` combinator to Stencils.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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

Thanks for the review!



================
Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:181
+/// This supports user-defined extensions to the Stencil language.
+StencilPart run(MatchConsumer<std::string> C);
+
----------------
gribozavr wrote:
> ymandel wrote:
> > gribozavr wrote:
> > > We could reimplement all other stencils through `run()` and eliminate `StencilPartInterface`. 
> > > 
> > > The idea is to make `StencilPart` contain a `std::shared_ptr<MatchConsumer<std::string>>`. Then `run()` can be implemented as creating a `StencilPart` directly, and everything else can be implemented in terms of `run()`.
> > > 
> > > WDYT?
> > Answering both questions together: the equality function is designed to make testing of the format-string parser feasible (or, at least, reasonable).  As is, the only reason not to do what you suggest above is exactly the equality function -- otherwise, StencilPart is just `std::shared_ptr<MatchConsumer<std::string>>`.
> > 
> > I can also plausibly imagine that we'll extend the interface to include a "print" function as well at some point.  
> > 
> > However, currently the parser hasn't been upstreamed and the print function is only an idea, so it would be reasonable to drop `StencilPartInterface` and all the associated baggage and we could reinstate it later as needed.  Or, if you can think of a good way to test the parser that doesn't require an equality function, that too would be convincing.
> Ilya suggested that rather than comparing objects, a better way is to compare pretty-printed strings. First, it avoids the need for equality (which might get used for other things), and second, test failures will be more debuggable because we will have an obvious diff in the pretty printed string.
> 
> With that, I agree that `StencilPartInterface` has to stay since we are going to have at least two operations.
Yes, I like that!  Pretty printing has obvious other benefits, which equality doesn't.  Will do in followup patch. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67969