[LibTooling] Add insert/remove convenience functions for creating `ASTEdit`s.

[ymandel]

Summary

change() is an all purpose function; the revision adds simple shortcuts for the specific operations of inserting (before/after) or removing source.

[ymandel]

added comments.

[ilya-biryukov]

Are there any corner cases with handling whitespace that we might want to handle or document them if we don't handle them?
E.g. if the range ends up being a name of an expanded macro , it's probably very easy to glue the macro name with whatever you're inserting.

#define FOO 1+2;
#define BAR 2+3;

[[FOO]] // [[ ]] marks the selected range

would insertBefore([[FOO]], "BAR") end up being BARFOO or BAR FOO?

clang/include/clang/Tooling/Refactoring/Transformer.h

202 ↗

(On Diff #202026)

Could you elaborate on what calculating the end location of \p S means?
Is the fact that the end location is calculated specific to this function, rather than RangeSelector in general?

The comment is very different from insertBefore, suggesting the function behaves differently in practice.

[ilya-biryukov]

I guess the question also applies to change, although it did not occur to me before.

[ymandel]

Thanks for the review.

In D62621#1528714, @ilya-biryukov wrote:

Are there any corner cases with handling whitespace that we might want to handle or document them if we don't handle them?
E.g. if the range ends up being a name of an expanded macro , it's probably very easy to glue the macro name with whatever you're inserting.

#define FOO 1+2;
#define BAR 2+3;

[[FOO]] // [[ ]] marks the selected range

would insertBefore([[FOO]], "BAR") end up being BARFOO or BAR FOO?

In D62621#1528716, @ilya-biryukov wrote:

I guess the question also applies to change, although it did not occur to me before.

Good questions. The answer to both is that it depends on the RangeSelector used. change itself takes exactly the range given it and modifies the source at that range. Since these APIs are acting at the character level, I think it could be confusing to add token-level reasoning. That said, we could add RangeSelectors and/or TextGenerators that are smarter about this and ensure/add spaces around text as needed. Since all changes should (in my opinion) be run through clang-format afterwards, there's no risk in adding extra whitespace. For example, we could change text() to add whitespace on both sides of the argument, or add a new combinator pad which does that (and leave text() alone). So, for this example, the user would write insertBefore([[FOO]], pad("BAR")) and get BAR FOO.

We could similarly update Stencil to pad its output by default or somesuch.

WDYT?

clang/include/clang/Tooling/Refactoring/Transformer.h

202 ↗

(On Diff #202026)

I'm glad you bring this up, since I struggled with how best to express this. The point is that it handles the difference between CharRange and TokenRange and does the "right" thing. This kind of thing has bitten me and others in the past -- it's a pretty typical newbie error to mishandle the SourceRange of a node.

That said, it is *not* doing anything more than RangeSelector already does, since RangeSelector is already built on CharSourceRange rather than SourceRange. So, I'd be fine dropping this mention entirely, since it's really a property of the whole system. That is, I could replace with
"Inserts \p Replacement after \p S, leaving the source selected by \S unchanged."

WDYT?

As for insertBefore -- since the beginning of a range is unambiguous, there's no need to say anything interesting there.

[ilya-biryukov]

In D62621#1529691, @ymandel wrote:

Thanks for the review.

Good questions. The answer to both is that it depends on the RangeSelector used. change itself takes exactly the range given it and modifies the source at that range. Since these APIs are acting at the character level, I think it could be confusing to add token-level reasoning. That said, we could add RangeSelectors and/or TextGenerators that are smarter about this and ensure/add spaces around text as needed. Since all changes should (in my opinion) be run through clang-format afterwards, there's no risk in adding extra whitespace. For example, we could change text() to add whitespace on both sides of the argument, or add a new combinator pad which does that (and leave text() alone). So, for this example, the user would write insertBefore([[FOO]], pad("BAR")) and get BAR FOO.

We could similarly update Stencil to pad its output by default or somesuch.

WDYT?

The current design sounds reasonable to me. The macro case I mentioned is somewhat weird and I'm not even sure how often it shows up in practice. Most range selectors based on AST nodes should start and end at reasonable position that don't require any extra spaces when replaced.

Let's keep the whitespace problem in mind, but not do anything about it just yet. If this actually turns out to be a problem in practice we can revisit, having some intuition what the actual tricky cases are and maybe with more approaches on how to solve them.

Also LGTM, thanks!

clang/include/clang/Tooling/Refactoring/Transformer.h

202 ↗

(On Diff #202026)

I like the idea of a short comment not mentioning the token range peculiarities. At the very end, that's what everyone should expect from the API. The fact that clang's methods require this is surprising, transformers just "boringly" do the right thing.

[ymandel]

adjusted API comments.

cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h

 /// \code
 ///   makeRule(callExpr(callee(functionDecl(hasName("foo")))),
 ///            change(text("bar()")))
 /// \endcode
 inline ASTEdit change(TextGenerator Replacement) {
   return change(node(RewriteRule::RootID), std::move(Replacement));
 }
 
+/// Inserts \p Replacement before \p S, leaving the source selected by \S
+/// unchanged.
+inline ASTEdit insertBefore(RangeSelector S, TextGenerator Replacement) {
+  return change(before(std::move(S)), std::move(Replacement));
+}
+
+/// Inserts \p Replacement after \p S, leaving the source selected by \S
+/// unchanged.
+inline ASTEdit insertAfter(RangeSelector S, TextGenerator Replacement) {
+  return change(after(std::move(S)), std::move(Replacement));
+}
+
+/// Removes the source selected by \p S.
+inline ASTEdit remove(RangeSelector S) {
+  return change(std::move(S), text(""));
+}
+
 /// The following three functions are a low-level part of the RewriteRule
 /// API. We expose them for use in implementing the fixtures that interpret
 /// RewriteRule, like Transformer and TransfomerTidy, or for more advanced
 /// users.
 //
 // FIXME: These functions are really public, if advanced, elements of the
 // RewriteRule API.  Recast them as such.  Or, just declare these functions
 // public and well-supported and move them out of `detail`.

cfe/trunk/unittests/Tooling/TransformerTest.cpp

TEST_F(TransformerTest, NodePartMemberMultiToken) {
   )cc";
 
   StringRef MemExpr = "member";
   testRule(makeRule(memberExpr().bind(MemExpr),
                     change(member(MemExpr), text("good"))),
            Input, Expected);
 }
 
+TEST_F(TransformerTest, InsertBeforeEdit) {
+  std::string Input = R"cc(
+    int f() {
+      return 7;
+    }
+  )cc";
+  std::string Expected = R"cc(
+    int f() {
+      int y = 3;
+      return 7;
+    }
+  )cc";
+
+  StringRef Ret = "return";
+  testRule(makeRule(returnStmt().bind(Ret),
+                    insertBefore(statement(Ret), text("int y = 3;"))),
+           Input, Expected);
+}
+
+TEST_F(TransformerTest, InsertAfterEdit) {
+  std::string Input = R"cc(
+    int f() {
+      int x = 5;
+      return 7;
+    }
+  )cc";
+  std::string Expected = R"cc(
+    int f() {
+      int x = 5;
+      int y = 3;
+      return 7;
+    }
+  )cc";
+
+  StringRef Decl = "decl";
+  testRule(makeRule(declStmt().bind(Decl),
+                    insertAfter(statement(Decl), text("int y = 3;"))),
+           Input, Expected);
+}
+
+TEST_F(TransformerTest, RemoveEdit) {
+  std::string Input = R"cc(
+    int f() {
+      int x = 5;
+      return 7;
+    }
+  )cc";
+  std::string Expected = R"cc(
+    int f() {
+      return 7;
+    }
+  )cc";
+
+  StringRef Decl = "decl";
+  testRule(makeRule(declStmt().bind(Decl), remove(statement(Decl))), Input,
+           Expected);
+}
+
 TEST_F(TransformerTest, MultiChange) {
   std::string Input = R"cc(
     void foo() {
       if (10 > 1.0)
         log(1) << "oh no!";
       else
         log(0) << "ok";
     }