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

[ymandel]

Summary

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.

[gribozavr2]

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?

[ymandel]

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")))

[gribozavr2]

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.

[ymandel]

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.

[gribozavr2]

Are you suggesting we remove text and selection entirely?

Yes!

[ymandel]

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.

clang/include/clang/Tooling/Transformer/Stencil.h

 Stencil addressOf(llvm::StringRef ExprId);
 
 /// Constructs a `MemberExpr` that accesses the named member (\p Member) of the
 /// object bound to \p BaseId. The access is constructed idiomatically: if \p
 /// BaseId is bound to `e` and \p Member identifies member `m`, then returns
 /// `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 <typename T> Stencil access(llvm::StringRef BaseId, T &&Member) {
+  return access(BaseId, makeStencil(std::forward<T>(Member)));
 }
 
 /// Chooses between the two stencil parts, based on whether \p ID is bound in
 /// the match.
 Stencil ifBound(llvm::StringRef Id, Stencil TrueStencil, Stencil FalseStencil);
 
 /// Chooses between the two strings, based on whether \p ID is bound in the
 /// match.

clang/unittests/Tooling/StencilTest.cpp

 
 TEST(StencilToStringTest, AccessOpText) {
   auto S = access("Id", "memberData");
   StringRef Expected = R"repr(access("Id", "memberData"))repr";
   EXPECT_EQ(S->toString(), Expected);
 }
 
 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);
 }
 
 TEST(StencilToStringTest, AccessOpStencil) {
   auto S = access("Id", cat("foo_", "bar"));
   StringRef Expected = R"repr(access("Id", seq("foo_", "bar")))repr";
   EXPECT_EQ(S->toString(), Expected);