[PATCH] D69625: [libTooling] Support implicit coercions in Stencil's `access` combinator.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D69625#1730324 , @gribozavr2 wrote:

> > Are you suggesting we remove text and selection entirely?
>
> Yes!


Sounds good. I'll do that in a separate patch. I think we can abandon this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69625



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

[PATCH] D69625: [libTooling] Support implicit coercions in Stencil's `access` combinator.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

> Are you suggesting we remove text and selection entirely?

Yes!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69625



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

[PATCH] D69625: [libTooling] Support implicit coercions in Stencil's `access` combinator.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D69625#1729702 , @gribozavr2 wrote:

> > Which idiom do you think we should encourage, then, for text and 
> > range-selectors -- the named combinator or the single-argument cat? That is
>
> You are making a very interesting point!
>
> It sounds very appealing to me to remove the special `text` and `selection` 
> functions in favor of `cat`. Users of the library (both readers and writers) 
> will have to know about `cat`. So the advantage of having special functions 
> `text` and `selection` is really limited I think.


Are you suggesting we remove `text` and `selection` entirely?  I really like 
this idea, just want to be sure I hadn't read more into your comment than you 
intended.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69625



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

[PATCH] D69625: [libTooling] Support implicit coercions in Stencil's `access` combinator.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

> Which idiom do you think we should encourage, then, for text and 
> range-selectors -- the named combinator or the single-argument cat? That is

You are making a very interesting point!

It sounds very appealing to me to remove the special `text` and `selection` 
functions in favor of `cat`. Users of the library (both readers and writers) 
will have to know about `cat`. So the advantage of having special functions 
`text` and `selection` is really limited I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69625



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

[PATCH] D69625: [libTooling] Support implicit coercions in Stencil's `access` combinator.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D69625#1729634 , @gribozavr2 wrote:

> I fully agree about passing a Stencil to `access`. However whether to call 
> `makeStencil` inside is an interesting question. On one hand, such implicit 
> conversions increase the convenience. On the other, they increase the API 
> surface (more possible argument types), and makes the API harder to read. 
> What does `access` take? "I don't know, some T" vs. "A Stencil".
>
> I think that implicit conversions for `cat` arguments can be justified 
> because it seems like `cat` will be used frequently; however, `access` won't 
> be as frequently called.
>
> What do you think?


This seems reasonable, particularly the problem caused by using a template. I'd 
consider having three explicit overloads, but this doesn't scale in general 
(especially once you have a combinator with 2+ Stencil args).

Which idiom do you think we should encourage, then, for text and 
range-selectors -- the named combinator or the single-argument `cat`? That is

  access("object", text("field"))
  access("object", selection(member("e")))

versus

  access("object", cat("field"))
  access("object", cat(member("e")))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69625



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

[PATCH] D69625: [libTooling] Support implicit coercions in Stencil's `access` combinator.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

I fully agree about passing a Stencil to `access`. However whether to call 
`makeStencil` inside is an interesting question. On one hand, such implicit 
conversions increase the convenience. On the other, they increase the API 
surface (more possible argument types), and makes the API harder to read. What 
does `access` take? "I don't know, some T" vs. "A Stencil".

I think that implicit conversions for `cat` arguments can be justified because 
it seems like `cat` will be used frequently; however, `access` won't be as 
frequently called.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69625



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

[PATCH] D69625: [libTooling] Support implicit coercions in Stencil's `access` combinator.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added a reviewer: gribozavr.
Herald added a project: clang.
ymandel added a parent revision: D69613: [libTooling] Simplify type structure 
of `Stencil`s..

Changes `clang::transformer::access` to also support `RangeSelector` as the
second argument.  This change makes `access` consistent with `cat` in that it
will accept text, `RangeSelector` or `Stencil`. The plan is for all Stencil
arguments to be supported in this way to provide a uniform user experience for
Stencil arguments.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69625

Files:
  clang/include/clang/Tooling/Transformer/Stencil.h
  clang/unittests/Tooling/StencilTest.cpp


Index: clang/unittests/Tooling/StencilTest.cpp
===
--- clang/unittests/Tooling/StencilTest.cpp
+++ clang/unittests/Tooling/StencilTest.cpp
@@ -396,7 +396,7 @@
 }
 
 TEST(StencilToStringTest, AccessOpSelector) {
-  auto S = access("Id", selection(name("otherId")));
+  auto S = access("Id", name("otherId"));
   StringRef Expected = R"repr(access("Id", selection(...)))repr";
   EXPECT_EQ(S->toString(), Expected);
 }
Index: clang/include/clang/Tooling/Transformer/Stencil.h
===
--- clang/include/clang/Tooling/Transformer/Stencil.h
+++ clang/include/clang/Tooling/Transformer/Stencil.h
@@ -112,8 +112,8 @@
 /// `e->m`, when e is a pointer, `e2->m` when e = `*e2` and `e.m` otherwise.
 /// Additionally, `e` is wrapped in parentheses, if needed.
 Stencil access(llvm::StringRef BaseId, Stencil Member);
-inline Stencil access(llvm::StringRef BaseId, llvm::StringRef Member) {
-  return access(BaseId, text(Member));
+template  Stencil access(llvm::StringRef BaseId, T &) {
+  return access(BaseId, makeStencil(std::forward(Member)));
 }
 
 /// Chooses between the two stencil parts, based on whether \p ID is bound in


Index: clang/unittests/Tooling/StencilTest.cpp
===
--- clang/unittests/Tooling/StencilTest.cpp
+++ clang/unittests/Tooling/StencilTest.cpp
@@ -396,7 +396,7 @@
 }
 
 TEST(StencilToStringTest, AccessOpSelector) {
-  auto S = access("Id", selection(name("otherId")));
+  auto S = access("Id", name("otherId"));
   StringRef Expected = R"repr(access("Id", selection(...)))repr";
   EXPECT_EQ(S->toString(), Expected);
 }
Index: clang/include/clang/Tooling/Transformer/Stencil.h
===
--- clang/include/clang/Tooling/Transformer/Stencil.h
+++ clang/include/clang/Tooling/Transformer/Stencil.h
@@ -112,8 +112,8 @@
 /// `e->m`, when e is a pointer, `e2->m` when e = `*e2` and `e.m` otherwise.
 /// Additionally, `e` is wrapped in parentheses, if needed.
 Stencil access(llvm::StringRef BaseId, Stencil Member);
-inline Stencil access(llvm::StringRef BaseId, llvm::StringRef Member) {
-  return access(BaseId, text(Member));
+template  Stencil access(llvm::StringRef BaseId, T &) {
+  return access(BaseId, makeStencil(std::forward(Member)));
 }
 
 /// Chooses between the two stencil parts, based on whether \p ID is bound in
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits