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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to