This is an archive of the discontinued LLVM Phabricator instance.

[libTooling] Add various Stencil combinators for expressions.
ClosedPublic

Authored by ymandel on Oct 1 2019, 8:08 PM.

Details

Summary

This revision adds three new Stencil combinators:

  • expression, which idiomatically constructs the source for an expression, including wrapping the expression's source in parentheses if needed.
  • deref, which constructs an idiomatic dereferencing expression.
  • addressOf, which constructs an idiomatic address-taking expression.

Event Timeline

ymandel created this revision.Oct 1 2019, 8:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2019, 8:08 PM
gribozavr accepted this revision.Oct 2 2019, 8:15 AM
gribozavr added inline comments.
clang/include/clang/Tooling/Refactoring/Stencil.h
149

Should the comment cross-reference expression() and say that the user probably wants that instead?

clang/lib/Tooling/Refactoring/Stencil.cpp
71

Add "unary" to the name somewhere?

This revision is now accepted and ready to land.Oct 2 2019, 8:15 AM
ymandel updated this revision to Diff 222889.Oct 2 2019, 12:22 PM
ymandel marked 2 inline comments as done.

responded to comments

ymandel marked an inline comment as done.Oct 2 2019, 12:23 PM
ymandel added inline comments.
clang/include/clang/Tooling/Refactoring/Stencil.h
149

That depends on what selector they're using. For selection(node(ExprId)), yes I think that expression(ExprId) is going to be better in most cases. But, for other selectors, no. So, I'm not sure that the cross-reference will be generally useful. WDYT?

Also, it occurs to me that we have an asymmetry for statements and expressions. Getting the source of a statement is
selection(statement(Id)) versus expression(Id) for expressions. However, in the context of cat, which takes RangeSelector directly, they look the same, because selection isn't needed.

gribozavr added inline comments.Oct 2 2019, 12:32 PM
clang/include/clang/Tooling/Refactoring/Stencil.h
149

Yeah, you're right -- there are different cases, and selection() is not just for expressions. Maybe just point out that digging out raw source code should be considered a fallback option, and other AST-aware and context-aware stencils should be preferred where they exist.

ymandel marked an inline comment as done.Oct 2 2019, 3:05 PM
ymandel added inline comments.
clang/include/clang/Tooling/Refactoring/Stencil.h
149

I'm still not sure what advice we want to give. In general, selection() is the way to go any time you want to propagate code from source to the target, particularly for cases where there is no corresponding node. So, in many cases it will be the only (and correct) option, rather than a fallback.

So, I'm inclined to leave it as is. The next major task I'm working on for Transformer is to write documentation for the set of related langauges here (that is, user-manual style documentation). I think that effort should help clarify what we want in some of these comments. WDYT?

gribozavr marked an inline comment as done.Oct 2 2019, 3:26 PM
gribozavr added inline comments.
clang/include/clang/Tooling/Refactoring/Stencil.h
149

Okay, probably I don't understand it sufficiently then. Feel free to leave it as is, especially since this comment was there before this patch.

This revision was automatically updated to reflect the committed changes.
ymandel marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2019, 5:59 AM