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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
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);
+
----------------
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?


================
Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:107
+// Equality is not defined over MatchConsumers, which are opaque.
+bool isEqualData(const MatchConsumer<std::string> &A,
+                 const MatchConsumer<std::string> &B) {
----------------
Not very appropriate for this review -- but why do Stencils support equality comparison? As you note, with things like `run` it makes little sense to support it as an API. However, since a user does not know whether a Stencil can contain `run` or not, it is not very meaningful to compare them at all.

So if equality is not used for anything important, consider removing it?

Not to even mention that as implemented, equality is not reflexive.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67969