[PATCH] D69184: [libTooling] Introduce general combinator for fixed-value `MatchConsumer`s.

Yitzhak Mandelbaum via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Oct 23 05:56:16 PDT 2019


ymandel added a comment.

In D69184#1718427 <https://reviews.llvm.org/D69184#1718427>, @ymandel wrote:

> In D69184#1715256 <https://reviews.llvm.org/D69184#1715256>, @gribozavr wrote:
>
> > What are the use cases for non-text values?
> >
> > I like the name `text` much better... If the name conflict is the issue, I think you could rename it to `toText` -- it would even read more fluently `change(toText("abc"))`, also making it obvious that we are not changing *the text abc*, but we are changing the code to say "abc".
>
>
> Another example use is: https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Tooling/Transformer/RangeSelector.h#L29-L32, although I must admit that I hadn't thought of that before this patch.  It's just the natural generalization (it's actually the K combinator form SKI fame).  So, I haven't thought about other examples -- the primary motivation was just the naming.
>
> That said, I'm not sure about `toText` because I consider the "to" as implicit in `change`. In fact, it might be a good idea to rename `change` to `changeTo` (WDYT?).  What about any of `asText`, `textMsg`, `textMessage`? (I realize the unfortunate pun in the last two, but not sure that's worth caring about).


An alternative, is not to introduce a new combinator at all, and just use `Stencil`s:

  change(cat("abc"))

Or, add `operator()` to `StencilPart` and use those directly:

  change(text("abc"))

Given that `Stencil` and `Transformer` are bundled in the same library, is there any strong reason for a client to avoid a dependency on `Stencil`?

  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69184





More information about the cfe-commits mailing list