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