This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ymandel on Oct 30 2019, 9:55 AM.

Details

Reviewers
gribozavr
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.

Event Timeline

ymandel created this revision.Oct 30 2019, 9:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2019, 9:55 AM

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?

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

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.

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.

Are you suggesting we remove text and selection entirely?

Yes!

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.

ymandel abandoned this revision.Nov 7 2019, 5:30 AM