[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL302624: Add ASTMatchRefactorer and ReplaceNodeWithTemplate 
to RefactoringCallbacks (authored by ioeric).

Changed prior to commit:
  https://reviews.llvm.org/D29621?vs=98203=98412#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D29621

Files:
  cfe/trunk/include/clang/Tooling/RefactoringCallbacks.h
  cfe/trunk/lib/Tooling/RefactoringCallbacks.cpp
  cfe/trunk/unittests/Tooling/RefactoringCallbacksTest.cpp

Index: cfe/trunk/include/clang/Tooling/RefactoringCallbacks.h
===
--- cfe/trunk/include/clang/Tooling/RefactoringCallbacks.h
+++ cfe/trunk/include/clang/Tooling/RefactoringCallbacks.h
@@ -47,6 +47,32 @@
   Replacements Replace;
 };
 
+/// \brief Adaptor between \c ast_matchers::MatchFinder and \c
+/// tooling::RefactoringTool.
+///
+/// Runs AST matchers and stores the \c tooling::Replacements in a map.
+class ASTMatchRefactorer {
+public:
+  ASTMatchRefactorer(std::map );
+
+  template 
+  void addMatcher(const T , RefactoringCallback *Callback) {
+MatchFinder.addMatcher(Matcher, Callback);
+Callbacks.push_back(Callback);
+  }
+
+  void addDynamicMatcher(const ast_matchers::internal::DynTypedMatcher ,
+ RefactoringCallback *Callback);
+
+  std::unique_ptr newASTConsumer();
+
+private:
+  friend class RefactoringASTConsumer;
+  std::vector Callbacks;
+  ast_matchers::MatchFinder MatchFinder;
+  std::map 
+};
+
 /// \brief Replace the text of the statement bound to \c FromId with the text in
 /// \c ToText.
 class ReplaceStmtWithText : public RefactoringCallback {
@@ -59,6 +85,29 @@
   std::string ToText;
 };
 
+/// \brief Replace the text of an AST node bound to \c FromId with the result of
+/// evaluating the template in \c ToTemplate.
+///
+/// Expressions of the form ${NodeName} in \c ToTemplate will be
+/// replaced by the text of the node bound to ${NodeName}. The string
+/// "$$" will be replaced by "$".
+class ReplaceNodeWithTemplate : public RefactoringCallback {
+public:
+  static llvm::Expected
+  create(StringRef FromId, StringRef ToTemplate);
+  void run(const ast_matchers::MatchFinder::MatchResult ) override;
+
+private:
+  struct TemplateElement {
+enum { Literal, Identifier } Type;
+std::string Value;
+  };
+  ReplaceNodeWithTemplate(llvm::StringRef FromId,
+  std::vector &);
+  std::string FromId;
+  std::vector Template;
+};
+
 /// \brief Replace the text of the statement bound to \c FromId with the text of
 /// the statement bound to \c ToId.
 class ReplaceStmtWithStmt : public RefactoringCallback {
Index: cfe/trunk/lib/Tooling/RefactoringCallbacks.cpp
===
--- cfe/trunk/lib/Tooling/RefactoringCallbacks.cpp
+++ cfe/trunk/lib/Tooling/RefactoringCallbacks.cpp
@@ -9,8 +9,13 @@
 //
 //
 //===--===//
-#include "clang/Lex/Lexer.h"
 #include "clang/Tooling/RefactoringCallbacks.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/Lexer.h"
+
+using llvm::StringError;
+using llvm::make_error;
 
 namespace clang {
 namespace tooling {
@@ -20,18 +25,62 @@
   return Replace;
 }
 
-static Replacement replaceStmtWithText(SourceManager ,
-   const Stmt ,
+ASTMatchRefactorer::ASTMatchRefactorer(
+std::map )
+: FileToReplaces(FileToReplaces) {}
+
+void ASTMatchRefactorer::addDynamicMatcher(
+const ast_matchers::internal::DynTypedMatcher ,
+RefactoringCallback *Callback) {
+  MatchFinder.addDynamicMatcher(Matcher, Callback);
+  Callbacks.push_back(Callback);
+}
+
+class RefactoringASTConsumer : public ASTConsumer {
+public:
+  RefactoringASTConsumer(ASTMatchRefactorer )
+  : Refactoring(Refactoring) {}
+
+  void HandleTranslationUnit(ASTContext ) override {
+// The ASTMatchRefactorer is re-used between translation units.
+// Clear the matchers so that each Replacement is only emitted once.
+for (const auto  : Refactoring.Callbacks) {
+  Callback->getReplacements().clear();
+}
+Refactoring.MatchFinder.matchAST(Context);
+for (const auto  : Refactoring.Callbacks) {
+  for (const auto  : Callback->getReplacements()) {
+llvm::Error Err =
+Refactoring.FileToReplaces[Replacement.getFilePath()].add(
+Replacement);
+if (Err) {
+  llvm::errs() << "Skipping replacement " << Replacement.toString()
+   << " due to this error:\n"
+   << toString(std::move(Err)) << "\n";
+}
+  }
+}
+  }
+
+private:
+  ASTMatchRefactorer 
+};
+
+std::unique_ptr ASTMatchRefactorer::newASTConsumer() {
+  return llvm::make_unique(*this);
+}
+

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert updated this revision to Diff 98203.
jbangert added a comment.

Ran check-clang


https://reviews.llvm.org/D29621

Files:
  include/clang/Tooling/RefactoringCallbacks.h
  lib/Tooling/RefactoringCallbacks.cpp
  unittests/Tooling/RefactoringCallbacksTest.cpp

Index: unittests/Tooling/RefactoringCallbacksTest.cpp
===
--- unittests/Tooling/RefactoringCallbacksTest.cpp
+++ unittests/Tooling/RefactoringCallbacksTest.cpp
@@ -7,31 +7,30 @@
 //
 //===--===//
 
-#include "clang/Tooling/RefactoringCallbacks.h"
 #include "RewriterTestContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/RefactoringCallbacks.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tooling {
 
 using namespace ast_matchers;
 
 template 
-void expectRewritten(const std::string ,
- const std::string ,
- const T ,
- RefactoringCallback ) {
-  MatchFinder Finder;
+void expectRewritten(const std::string , const std::string ,
+ const T , RefactoringCallback ) {
+  std::map FileToReplace;
+  ASTMatchRefactorer Finder(FileToReplace);
   Finder.addMatcher(AMatcher, );
   std::unique_ptr Factory(
   tooling::newFrontendActionFactory());
   ASSERT_TRUE(tooling::runToolOnCode(Factory->create(), Code))
   << "Parsing error in \"" << Code << "\"";
   RewriterTestContext Context;
   FileID ID = Context.createInMemoryFile("input.cc", Code);
-  EXPECT_TRUE(tooling::applyAllReplacements(Callback.getReplacements(),
+  EXPECT_TRUE(tooling::applyAllReplacements(FileToReplace["input.cc"],
 Context.Rewrite));
   EXPECT_EQ(Expected, Context.getRewrittenText(ID));
 }
@@ -61,40 +60,94 @@
   std::string Code = "void f() { int i = 1; }";
   std::string Expected = "void f() { int i = 2; }";
   ReplaceStmtWithText Callback("id", "2");
-  expectRewritten(Code, Expected, id("id", expr(integerLiteral())),
-  Callback);
+  expectRewritten(Code, Expected, id("id", expr(integerLiteral())), Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesStmtWithStmt) {
   std::string Code = "void f() { int i = false ? 1 : i * 2; }";
   std::string Expected = "void f() { int i = i * 2; }";
   ReplaceStmtWithStmt Callback("always-false", "should-be");
-  expectRewritten(Code, Expected,
-  id("always-false", conditionalOperator(
-  hasCondition(cxxBoolLiteral(equals(false))),
-  hasFalseExpression(id("should-be", expr(),
+  expectRewritten(
+  Code, Expected,
+  id("always-false",
+ conditionalOperator(hasCondition(cxxBoolLiteral(equals(false))),
+ hasFalseExpression(id("should-be", expr(),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesIfStmt) {
   std::string Code = "bool a; void f() { if (a) f(); else a = true; }";
   std::string Expected = "bool a; void f() { f(); }";
   ReplaceIfStmtWithItsBody Callback("id", true);
-  expectRewritten(Code, Expected,
-  id("id", ifStmt(
-  hasCondition(implicitCastExpr(hasSourceExpression(
-  declRefExpr(to(varDecl(hasName("a"),
+  expectRewritten(
+  Code, Expected,
+  id("id", ifStmt(hasCondition(implicitCastExpr(hasSourceExpression(
+   declRefExpr(to(varDecl(hasName("a"),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, RemovesEntireIfOnEmptyElse) {
   std::string Code = "void f() { if (false) int i = 0; }";
   std::string Expected = "void f() {  }";
   ReplaceIfStmtWithItsBody Callback("id", false);
   expectRewritten(Code, Expected,
-  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
-  Callback);
+  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
+  Callback);
 }
 
+TEST(RefactoringCallbacksTest, TemplateJustText) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { FOO }";
+  auto Callback = ReplaceNodeWithTemplate::create("id", "FOO");
+  EXPECT_FALSE(Callback.takeError());
+  expectRewritten(Code, Expected, id("id", declStmt()), **Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateSimpleSubst) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { long x = 1; }";
+  auto Callback = ReplaceNodeWithTemplate::create("decl", "long x = ${init}");
+  EXPECT_FALSE(Callback.takeError());
+  expectRewritten(Code, Expected,
+  id("decl", varDecl(hasInitializer(id("init", expr(),
+  **Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateLiteral) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { string x = \"$-1\"; }";
+  auto Callback = ReplaceNodeWithTemplate::create("decl",
+   

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Still LGTM. Please run `ninja check-clang` and fix the warning. I'll land this 
patch for you afterwards.




Comment at: lib/Tooling/RefactoringCallbacks.cpp:222
+}
+default:
+  llvm_unreachable("Element.Type not recognized");

```
ninja check-clang
RefactoringCallbacks.cpp:222:5: warning: default label in switch which covers 
all enumeration values [-Wcovered-switch-default]
```
Please remove the `default`.



https://reviews.llvm.org/D29621



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert accepted this revision.
jbangert marked 2 inline comments as done.
jbangert added inline comments.



Comment at: unittests/Tooling/RefactoringCallbacksTest.cpp:101
+  std::string Expected = "void f() { FOO }";
+  ReplaceNodeWithTemplate Callback("id", "FOO");
+  expectRewritten(Code, Expected, id("id", declStmt()), Callback);

ioeric wrote:
> Have you rerun the tests? Does this still build?
ninja check-clang-tools works. 


https://reviews.llvm.org/D29621



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert updated this revision to Diff 94282.
jbangert added a comment.

- Merge branch 'master' of http://llvm.org/git/clang into replace_template


https://reviews.llvm.org/D29621

Files:
  include/clang/Tooling/RefactoringCallbacks.h
  lib/Tooling/RefactoringCallbacks.cpp
  unittests/Tooling/RefactoringCallbacksTest.cpp

Index: unittests/Tooling/RefactoringCallbacksTest.cpp
===
--- unittests/Tooling/RefactoringCallbacksTest.cpp
+++ unittests/Tooling/RefactoringCallbacksTest.cpp
@@ -7,31 +7,30 @@
 //
 //===--===//
 
-#include "clang/Tooling/RefactoringCallbacks.h"
 #include "RewriterTestContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/RefactoringCallbacks.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tooling {
 
 using namespace ast_matchers;
 
 template 
-void expectRewritten(const std::string ,
- const std::string ,
- const T ,
- RefactoringCallback ) {
-  MatchFinder Finder;
+void expectRewritten(const std::string , const std::string ,
+ const T , RefactoringCallback ) {
+  std::map FileToReplace;
+  ASTMatchRefactorer Finder(FileToReplace);
   Finder.addMatcher(AMatcher, );
   std::unique_ptr Factory(
   tooling::newFrontendActionFactory());
   ASSERT_TRUE(tooling::runToolOnCode(Factory->create(), Code))
   << "Parsing error in \"" << Code << "\"";
   RewriterTestContext Context;
   FileID ID = Context.createInMemoryFile("input.cc", Code);
-  EXPECT_TRUE(tooling::applyAllReplacements(Callback.getReplacements(),
+  EXPECT_TRUE(tooling::applyAllReplacements(FileToReplace["input.cc"],
 Context.Rewrite));
   EXPECT_EQ(Expected, Context.getRewrittenText(ID));
 }
@@ -61,40 +60,94 @@
   std::string Code = "void f() { int i = 1; }";
   std::string Expected = "void f() { int i = 2; }";
   ReplaceStmtWithText Callback("id", "2");
-  expectRewritten(Code, Expected, id("id", expr(integerLiteral())),
-  Callback);
+  expectRewritten(Code, Expected, id("id", expr(integerLiteral())), Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesStmtWithStmt) {
   std::string Code = "void f() { int i = false ? 1 : i * 2; }";
   std::string Expected = "void f() { int i = i * 2; }";
   ReplaceStmtWithStmt Callback("always-false", "should-be");
-  expectRewritten(Code, Expected,
-  id("always-false", conditionalOperator(
-  hasCondition(cxxBoolLiteral(equals(false))),
-  hasFalseExpression(id("should-be", expr(),
+  expectRewritten(
+  Code, Expected,
+  id("always-false",
+ conditionalOperator(hasCondition(cxxBoolLiteral(equals(false))),
+ hasFalseExpression(id("should-be", expr(),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesIfStmt) {
   std::string Code = "bool a; void f() { if (a) f(); else a = true; }";
   std::string Expected = "bool a; void f() { f(); }";
   ReplaceIfStmtWithItsBody Callback("id", true);
-  expectRewritten(Code, Expected,
-  id("id", ifStmt(
-  hasCondition(implicitCastExpr(hasSourceExpression(
-  declRefExpr(to(varDecl(hasName("a"),
+  expectRewritten(
+  Code, Expected,
+  id("id", ifStmt(hasCondition(implicitCastExpr(hasSourceExpression(
+   declRefExpr(to(varDecl(hasName("a"),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, RemovesEntireIfOnEmptyElse) {
   std::string Code = "void f() { if (false) int i = 0; }";
   std::string Expected = "void f() {  }";
   ReplaceIfStmtWithItsBody Callback("id", false);
   expectRewritten(Code, Expected,
-  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
-  Callback);
+  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
+  Callback);
 }
 
+TEST(RefactoringCallbacksTest, TemplateJustText) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { FOO }";
+  auto Callback = ReplaceNodeWithTemplate::create("id", "FOO");
+  EXPECT_FALSE(Callback.takeError());
+  expectRewritten(Code, Expected, id("id", declStmt()), **Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateSimpleSubst) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { long x = 1; }";
+  auto Callback = ReplaceNodeWithTemplate::create("decl", "long x = ${init}");
+  EXPECT_FALSE(Callback.takeError());
+  expectRewritten(Code, Expected,
+  id("decl", varDecl(hasInitializer(id("init", expr(),
+  **Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateLiteral) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { string x = \"$-1\"; }";
+  auto 

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert added inline comments.



Comment at: lib/Tooling/RefactoringCallbacks.cpp:213
+llvm::errs() << "Node " << Element.Value
+ << " used in replacement template not bound in Matcher 
\n";
+llvm_unreachable("Unbound node in replacement template.");

sbenza wrote:
> I don't know if stderr is the best place for this error output.
> Maybe we should take a sink of some sort in the constructor.
There's a FIXME: above that addresses the need for better error handling, and 
this one just duplicated their workaround. 
Ultimately, clang::tooling:: should probably provide infrastructure for 
reporting problems (I would imagine all refactoring tools have errors 
occassionally, and they need some way of reporting/handling them). 



Comment at: lib/Tooling/RefactoringCallbacks.cpp:214
+ << " used in replacement template not bound in Matcher 
\n";
+llvm_unreachable("Unbound node in replacement template.");
+  }

sbenza wrote:
> I don't think this is ok.
> afaik, llvm_unreachable leads to undefined behavior if it is reached in opt 
> mode.
> This error can be triggered from user input. We should not fail that way.
Using llvm::report_fatal_error for now. See the above for better error 
handling. 


https://reviews.llvm.org/D29621



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert updated this revision to Diff 89730.
jbangert marked an inline comment as done.
jbangert added a comment.

use llvm::report_fatal_error instead of unreachable.


https://reviews.llvm.org/D29621

Files:
  include/clang/Tooling/RefactoringCallbacks.h
  lib/Tooling/RefactoringCallbacks.cpp
  unittests/Tooling/RefactoringCallbacksTest.cpp

Index: unittests/Tooling/RefactoringCallbacksTest.cpp
===
--- unittests/Tooling/RefactoringCallbacksTest.cpp
+++ unittests/Tooling/RefactoringCallbacksTest.cpp
@@ -7,31 +7,30 @@
 //
 //===--===//
 
-#include "clang/Tooling/RefactoringCallbacks.h"
 #include "RewriterTestContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/RefactoringCallbacks.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tooling {
 
 using namespace ast_matchers;
 
 template 
-void expectRewritten(const std::string ,
- const std::string ,
- const T ,
- RefactoringCallback ) {
-  MatchFinder Finder;
+void expectRewritten(const std::string , const std::string ,
+ const T , RefactoringCallback ) {
+  std::map FileToReplace;
+  ASTMatchRefactorer Finder(FileToReplace);
   Finder.addMatcher(AMatcher, );
   std::unique_ptr Factory(
   tooling::newFrontendActionFactory());
   ASSERT_TRUE(tooling::runToolOnCode(Factory->create(), Code))
   << "Parsing error in \"" << Code << "\"";
   RewriterTestContext Context;
   FileID ID = Context.createInMemoryFile("input.cc", Code);
-  EXPECT_TRUE(tooling::applyAllReplacements(Callback.getReplacements(),
+  EXPECT_TRUE(tooling::applyAllReplacements(FileToReplace["input.cc"],
 Context.Rewrite));
   EXPECT_EQ(Expected, Context.getRewrittenText(ID));
 }
@@ -61,40 +60,94 @@
   std::string Code = "void f() { int i = 1; }";
   std::string Expected = "void f() { int i = 2; }";
   ReplaceStmtWithText Callback("id", "2");
-  expectRewritten(Code, Expected, id("id", expr(integerLiteral())),
-  Callback);
+  expectRewritten(Code, Expected, id("id", expr(integerLiteral())), Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesStmtWithStmt) {
   std::string Code = "void f() { int i = false ? 1 : i * 2; }";
   std::string Expected = "void f() { int i = i * 2; }";
   ReplaceStmtWithStmt Callback("always-false", "should-be");
-  expectRewritten(Code, Expected,
-  id("always-false", conditionalOperator(
-  hasCondition(cxxBoolLiteral(equals(false))),
-  hasFalseExpression(id("should-be", expr(),
+  expectRewritten(
+  Code, Expected,
+  id("always-false",
+ conditionalOperator(hasCondition(cxxBoolLiteral(equals(false))),
+ hasFalseExpression(id("should-be", expr(),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesIfStmt) {
   std::string Code = "bool a; void f() { if (a) f(); else a = true; }";
   std::string Expected = "bool a; void f() { f(); }";
   ReplaceIfStmtWithItsBody Callback("id", true);
-  expectRewritten(Code, Expected,
-  id("id", ifStmt(
-  hasCondition(implicitCastExpr(hasSourceExpression(
-  declRefExpr(to(varDecl(hasName("a"),
+  expectRewritten(
+  Code, Expected,
+  id("id", ifStmt(hasCondition(implicitCastExpr(hasSourceExpression(
+   declRefExpr(to(varDecl(hasName("a"),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, RemovesEntireIfOnEmptyElse) {
   std::string Code = "void f() { if (false) int i = 0; }";
   std::string Expected = "void f() {  }";
   ReplaceIfStmtWithItsBody Callback("id", false);
   expectRewritten(Code, Expected,
-  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
-  Callback);
+  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
+  Callback);
 }
 
+TEST(RefactoringCallbacksTest, TemplateJustText) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { FOO }";
+  auto Callback = ReplaceNodeWithTemplate::create("id", "FOO");
+  EXPECT_FALSE(Callback.takeError());
+  expectRewritten(Code, Expected, id("id", declStmt()), **Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateSimpleSubst) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { long x = 1; }";
+  auto Callback = ReplaceNodeWithTemplate::create("decl", "long x = ${init}");
+  EXPECT_FALSE(Callback.takeError());
+  expectRewritten(Code, Expected,
+  id("decl", varDecl(hasInitializer(id("init", expr(),
+  **Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateLiteral) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { string x = 

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: lib/Tooling/RefactoringCallbacks.cpp:213
+llvm::errs() << "Node " << Element.Value
+ << " used in replacement template not bound in Matcher 
\n";
+llvm_unreachable("Unbound node in replacement template.");

I don't know if stderr is the best place for this error output.
Maybe we should take a sink of some sort in the constructor.



Comment at: lib/Tooling/RefactoringCallbacks.cpp:214
+ << " used in replacement template not bound in Matcher 
\n";
+llvm_unreachable("Unbound node in replacement template.");
+  }

I don't think this is ok.
afaik, llvm_unreachable leads to undefined behavior if it is reached in opt 
mode.
This error can be triggered from user input. We should not fail that way.



Comment at: lib/Tooling/RefactoringCallbacks.cpp:227
+  if (NodeMap.count(FromId) == 0) {
+llvm::errs() << "Node to be replaced " << FromId
+ << " not bound in query.\n";

Same as above. This error can be triggered from user input.


https://reviews.llvm.org/D29621



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert accepted this revision.
jbangert added a comment.

Thanks, added tests for parser failures.


https://reviews.llvm.org/D29621



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert updated this revision to Diff 89420.
jbangert marked an inline comment as done.
jbangert added a comment.

- additional tests


https://reviews.llvm.org/D29621

Files:
  include/clang/Tooling/RefactoringCallbacks.h
  lib/Tooling/RefactoringCallbacks.cpp
  unittests/Tooling/RefactoringCallbacksTest.cpp

Index: unittests/Tooling/RefactoringCallbacksTest.cpp
===
--- unittests/Tooling/RefactoringCallbacksTest.cpp
+++ unittests/Tooling/RefactoringCallbacksTest.cpp
@@ -7,31 +7,30 @@
 //
 //===--===//
 
-#include "clang/Tooling/RefactoringCallbacks.h"
 #include "RewriterTestContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/RefactoringCallbacks.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tooling {
 
 using namespace ast_matchers;
 
 template 
-void expectRewritten(const std::string ,
- const std::string ,
- const T ,
- RefactoringCallback ) {
-  MatchFinder Finder;
+void expectRewritten(const std::string , const std::string ,
+ const T , RefactoringCallback ) {
+  std::map FileToReplace;
+  ASTMatchRefactorer Finder(FileToReplace);
   Finder.addMatcher(AMatcher, );
   std::unique_ptr Factory(
   tooling::newFrontendActionFactory());
   ASSERT_TRUE(tooling::runToolOnCode(Factory->create(), Code))
   << "Parsing error in \"" << Code << "\"";
   RewriterTestContext Context;
   FileID ID = Context.createInMemoryFile("input.cc", Code);
-  EXPECT_TRUE(tooling::applyAllReplacements(Callback.getReplacements(),
+  EXPECT_TRUE(tooling::applyAllReplacements(FileToReplace["input.cc"],
 Context.Rewrite));
   EXPECT_EQ(Expected, Context.getRewrittenText(ID));
 }
@@ -61,40 +60,94 @@
   std::string Code = "void f() { int i = 1; }";
   std::string Expected = "void f() { int i = 2; }";
   ReplaceStmtWithText Callback("id", "2");
-  expectRewritten(Code, Expected, id("id", expr(integerLiteral())),
-  Callback);
+  expectRewritten(Code, Expected, id("id", expr(integerLiteral())), Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesStmtWithStmt) {
   std::string Code = "void f() { int i = false ? 1 : i * 2; }";
   std::string Expected = "void f() { int i = i * 2; }";
   ReplaceStmtWithStmt Callback("always-false", "should-be");
-  expectRewritten(Code, Expected,
-  id("always-false", conditionalOperator(
-  hasCondition(cxxBoolLiteral(equals(false))),
-  hasFalseExpression(id("should-be", expr(),
+  expectRewritten(
+  Code, Expected,
+  id("always-false",
+ conditionalOperator(hasCondition(cxxBoolLiteral(equals(false))),
+ hasFalseExpression(id("should-be", expr(),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesIfStmt) {
   std::string Code = "bool a; void f() { if (a) f(); else a = true; }";
   std::string Expected = "bool a; void f() { f(); }";
   ReplaceIfStmtWithItsBody Callback("id", true);
-  expectRewritten(Code, Expected,
-  id("id", ifStmt(
-  hasCondition(implicitCastExpr(hasSourceExpression(
-  declRefExpr(to(varDecl(hasName("a"),
+  expectRewritten(
+  Code, Expected,
+  id("id", ifStmt(hasCondition(implicitCastExpr(hasSourceExpression(
+   declRefExpr(to(varDecl(hasName("a"),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, RemovesEntireIfOnEmptyElse) {
   std::string Code = "void f() { if (false) int i = 0; }";
   std::string Expected = "void f() {  }";
   ReplaceIfStmtWithItsBody Callback("id", false);
   expectRewritten(Code, Expected,
-  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
-  Callback);
+  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
+  Callback);
 }
 
+TEST(RefactoringCallbacksTest, TemplateJustText) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { FOO }";
+  auto Callback = ReplaceNodeWithTemplate::create("id", "FOO");
+  EXPECT_FALSE(Callback.takeError());
+  expectRewritten(Code, Expected, id("id", declStmt()), **Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateSimpleSubst) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { long x = 1; }";
+  auto Callback = ReplaceNodeWithTemplate::create("decl", "long x = ${init}");
+  EXPECT_FALSE(Callback.takeError());
+  expectRewritten(Code, Expected,
+  id("decl", varDecl(hasInitializer(id("init", expr(),
+  **Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateLiteral) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { string x = \"$-1\"; }";
+  auto Callback = 

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: include/clang/Tooling/RefactoringCallbacks.h:61
+MatchFinder.addMatcher(Matcher, Callback);
+Callbacks.emplace_back(Callback);
+  }

jbangert wrote:
> sbenza wrote:
> > Why emplace_back instead of push_back?
> Changed to push_back.  Is there ever an advantage to using push_back over 
> emplace_back (the latter falls back to a copy constructor). 
push_back and emplace_back are equivalent when the value you are passing is 
already a T.
In that case, push_back should be used from the principle of using the least 
powerful tool that can do the job you want.
emplace_back allows for calls to explicit constructors, which makes it more 
powerful in ways that might be surprising to the reader.
Eg:

std::vector v;
... many lines below ...
v.push_back(1);  // fails
v.emplace_back(1);  // works, but it didn't add a 1 to the vector.


https://reviews.llvm.org/D29621



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.

Looks good. Could you also add test cases where 
`ReplaceNodeWithTemplate::create` fails to parse templates?


https://reviews.llvm.org/D29621



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert updated this revision to Diff 87879.
jbangert added a comment.

- fix test


https://reviews.llvm.org/D29621

Files:
  include/clang/Tooling/RefactoringCallbacks.h
  lib/Tooling/RefactoringCallbacks.cpp
  unittests/Tooling/RefactoringCallbacksTest.cpp

Index: unittests/Tooling/RefactoringCallbacksTest.cpp
===
--- unittests/Tooling/RefactoringCallbacksTest.cpp
+++ unittests/Tooling/RefactoringCallbacksTest.cpp
@@ -7,31 +7,30 @@
 //
 //===--===//
 
-#include "clang/Tooling/RefactoringCallbacks.h"
 #include "RewriterTestContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/RefactoringCallbacks.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tooling {
 
 using namespace ast_matchers;
 
 template 
-void expectRewritten(const std::string ,
- const std::string ,
- const T ,
- RefactoringCallback ) {
-  MatchFinder Finder;
+void expectRewritten(const std::string , const std::string ,
+ const T , RefactoringCallback ) {
+  std::map FileToReplace;
+  ASTMatchRefactorer Finder(FileToReplace);
   Finder.addMatcher(AMatcher, );
   std::unique_ptr Factory(
   tooling::newFrontendActionFactory());
   ASSERT_TRUE(tooling::runToolOnCode(Factory->create(), Code))
   << "Parsing error in \"" << Code << "\"";
   RewriterTestContext Context;
   FileID ID = Context.createInMemoryFile("input.cc", Code);
-  EXPECT_TRUE(tooling::applyAllReplacements(Callback.getReplacements(),
+  EXPECT_TRUE(tooling::applyAllReplacements(FileToReplace["input.cc"],
 Context.Rewrite));
   EXPECT_EQ(Expected, Context.getRewrittenText(ID));
 }
@@ -61,39 +60,68 @@
   std::string Code = "void f() { int i = 1; }";
   std::string Expected = "void f() { int i = 2; }";
   ReplaceStmtWithText Callback("id", "2");
-  expectRewritten(Code, Expected, id("id", expr(integerLiteral())),
-  Callback);
+  expectRewritten(Code, Expected, id("id", expr(integerLiteral())), Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesStmtWithStmt) {
   std::string Code = "void f() { int i = false ? 1 : i * 2; }";
   std::string Expected = "void f() { int i = i * 2; }";
   ReplaceStmtWithStmt Callback("always-false", "should-be");
-  expectRewritten(Code, Expected,
-  id("always-false", conditionalOperator(
-  hasCondition(cxxBoolLiteral(equals(false))),
-  hasFalseExpression(id("should-be", expr(),
+  expectRewritten(
+  Code, Expected,
+  id("always-false",
+ conditionalOperator(hasCondition(cxxBoolLiteral(equals(false))),
+ hasFalseExpression(id("should-be", expr(),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesIfStmt) {
   std::string Code = "bool a; void f() { if (a) f(); else a = true; }";
   std::string Expected = "bool a; void f() { f(); }";
   ReplaceIfStmtWithItsBody Callback("id", true);
-  expectRewritten(Code, Expected,
-  id("id", ifStmt(
-  hasCondition(implicitCastExpr(hasSourceExpression(
-  declRefExpr(to(varDecl(hasName("a"),
+  expectRewritten(
+  Code, Expected,
+  id("id", ifStmt(hasCondition(implicitCastExpr(hasSourceExpression(
+   declRefExpr(to(varDecl(hasName("a"),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, RemovesEntireIfOnEmptyElse) {
   std::string Code = "void f() { if (false) int i = 0; }";
   std::string Expected = "void f() {  }";
   ReplaceIfStmtWithItsBody Callback("id", false);
   expectRewritten(Code, Expected,
-  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
-  Callback);
+  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
+  Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateJustText) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { FOO }";
+  auto Callback = ReplaceNodeWithTemplate::create("id", "FOO");
+  EXPECT_FALSE(Callback.takeError());
+  expectRewritten(Code, Expected, id("id", declStmt()), **Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateSimpleSubst) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { long x = 1; }";
+  auto Callback = ReplaceNodeWithTemplate::create("decl", "long x = ${init}");
+  EXPECT_FALSE(Callback.takeError());
+  expectRewritten(Code, Expected,
+  id("decl", varDecl(hasInitializer(id("init", expr(),
+  **Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateLiteral) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { string x = \"$-1\"; }";
+  auto Callback = ReplaceNodeWithTemplate::create("decl",
+

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert marked 2 inline comments as done.
jbangert added a comment.

Fixed the test -- I am still getting used to the different ninja test targets.




Comment at: lib/Tooling/RefactoringCallbacks.cpp:160
+llvm::Expected
+ReplaceNodeWithTemplate::create(StringRef FromId, StringRef ToTemplate) {
+  std::vector ParsedTemplate;

ioeric wrote:
> Is this covered in the test?
Now it is. 


https://reviews.llvm.org/D29621



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: lib/Tooling/RefactoringCallbacks.cpp:160
+llvm::Expected
+ReplaceNodeWithTemplate::create(StringRef FromId, StringRef ToTemplate) {
+  std::vector ParsedTemplate;

Is this covered in the test?



Comment at: unittests/Tooling/RefactoringCallbacksTest.cpp:101
+  std::string Expected = "void f() { FOO }";
+  ReplaceNodeWithTemplate Callback("id", "FOO");
+  expectRewritten(Code, Expected, id("id", declStmt()), Callback);

Have you rerun the tests? Does this still build?


https://reviews.llvm.org/D29621



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert updated this revision to Diff 87579.
jbangert added a comment.

- use iter


https://reviews.llvm.org/D29621

Files:
  include/clang/Tooling/RefactoringCallbacks.h
  lib/Tooling/RefactoringCallbacks.cpp
  unittests/Tooling/RefactoringCallbacksTest.cpp

Index: unittests/Tooling/RefactoringCallbacksTest.cpp
===
--- unittests/Tooling/RefactoringCallbacksTest.cpp
+++ unittests/Tooling/RefactoringCallbacksTest.cpp
@@ -7,31 +7,30 @@
 //
 //===--===//
 
-#include "clang/Tooling/RefactoringCallbacks.h"
 #include "RewriterTestContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/RefactoringCallbacks.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tooling {
 
 using namespace ast_matchers;
 
 template 
-void expectRewritten(const std::string ,
- const std::string ,
- const T ,
- RefactoringCallback ) {
-  MatchFinder Finder;
+void expectRewritten(const std::string , const std::string ,
+ const T , RefactoringCallback ) {
+  std::map FileToReplace;
+  ASTMatchRefactorer Finder(FileToReplace);
   Finder.addMatcher(AMatcher, );
   std::unique_ptr Factory(
   tooling::newFrontendActionFactory());
   ASSERT_TRUE(tooling::runToolOnCode(Factory->create(), Code))
   << "Parsing error in \"" << Code << "\"";
   RewriterTestContext Context;
   FileID ID = Context.createInMemoryFile("input.cc", Code);
-  EXPECT_TRUE(tooling::applyAllReplacements(Callback.getReplacements(),
+  EXPECT_TRUE(tooling::applyAllReplacements(FileToReplace["input.cc"],
 Context.Rewrite));
   EXPECT_EQ(Expected, Context.getRewrittenText(ID));
 }
@@ -61,39 +60,64 @@
   std::string Code = "void f() { int i = 1; }";
   std::string Expected = "void f() { int i = 2; }";
   ReplaceStmtWithText Callback("id", "2");
-  expectRewritten(Code, Expected, id("id", expr(integerLiteral())),
-  Callback);
+  expectRewritten(Code, Expected, id("id", expr(integerLiteral())), Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesStmtWithStmt) {
   std::string Code = "void f() { int i = false ? 1 : i * 2; }";
   std::string Expected = "void f() { int i = i * 2; }";
   ReplaceStmtWithStmt Callback("always-false", "should-be");
-  expectRewritten(Code, Expected,
-  id("always-false", conditionalOperator(
-  hasCondition(cxxBoolLiteral(equals(false))),
-  hasFalseExpression(id("should-be", expr(),
+  expectRewritten(
+  Code, Expected,
+  id("always-false",
+ conditionalOperator(hasCondition(cxxBoolLiteral(equals(false))),
+ hasFalseExpression(id("should-be", expr(),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesIfStmt) {
   std::string Code = "bool a; void f() { if (a) f(); else a = true; }";
   std::string Expected = "bool a; void f() { f(); }";
   ReplaceIfStmtWithItsBody Callback("id", true);
-  expectRewritten(Code, Expected,
-  id("id", ifStmt(
-  hasCondition(implicitCastExpr(hasSourceExpression(
-  declRefExpr(to(varDecl(hasName("a"),
+  expectRewritten(
+  Code, Expected,
+  id("id", ifStmt(hasCondition(implicitCastExpr(hasSourceExpression(
+   declRefExpr(to(varDecl(hasName("a"),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, RemovesEntireIfOnEmptyElse) {
   std::string Code = "void f() { if (false) int i = 0; }";
   std::string Expected = "void f() {  }";
   ReplaceIfStmtWithItsBody Callback("id", false);
   expectRewritten(Code, Expected,
-  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
-  Callback);
+  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
+  Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateJustText) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { FOO }";
+  ReplaceNodeWithTemplate Callback("id", "FOO");
+  expectRewritten(Code, Expected, id("id", declStmt()), Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateSimpleSubst) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { long x = 1; }";
+  ReplaceNodeWithTemplate Callback("decl", "long x = ${init}");
+  expectRewritten(Code, Expected,
+  id("decl", varDecl(hasInitializer(id("init", expr(),
+  Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateLiteral) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { string x = \"$-1\"; }";
+  ReplaceNodeWithTemplate Callback("decl", "string x = \"$$-${init}\"");
+  expectRewritten(Code, Expected,
+  id("decl", varDecl(hasInitializer(id("init", expr(),
+  

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert marked 5 inline comments as done.
jbangert added a comment.

Thank you for the initial feedback!




Comment at: include/clang/Tooling/RefactoringCallbacks.h:61
+MatchFinder.addMatcher(Matcher, Callback);
+Callbacks.emplace_back(Callback);
+  }

sbenza wrote:
> Why emplace_back instead of push_back?
Changed to push_back.  Is there ever an advantage to using push_back over 
emplace_back (the latter falls back to a copy constructor). 


https://reviews.llvm.org/D29621



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert marked 2 inline comments as done.
jbangert added a comment.

(marking other comments as done).


https://reviews.llvm.org/D29621



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert updated this revision to Diff 87575.
jbangert added a comment.

- change to push_back


https://reviews.llvm.org/D29621

Files:
  include/clang/Tooling/RefactoringCallbacks.h
  lib/Tooling/RefactoringCallbacks.cpp
  unittests/Tooling/RefactoringCallbacksTest.cpp

Index: unittests/Tooling/RefactoringCallbacksTest.cpp
===
--- unittests/Tooling/RefactoringCallbacksTest.cpp
+++ unittests/Tooling/RefactoringCallbacksTest.cpp
@@ -7,31 +7,30 @@
 //
 //===--===//
 
-#include "clang/Tooling/RefactoringCallbacks.h"
 #include "RewriterTestContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/RefactoringCallbacks.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tooling {
 
 using namespace ast_matchers;
 
 template 
-void expectRewritten(const std::string ,
- const std::string ,
- const T ,
- RefactoringCallback ) {
-  MatchFinder Finder;
+void expectRewritten(const std::string , const std::string ,
+ const T , RefactoringCallback ) {
+  std::map FileToReplace;
+  ASTMatchRefactorer Finder(FileToReplace);
   Finder.addMatcher(AMatcher, );
   std::unique_ptr Factory(
   tooling::newFrontendActionFactory());
   ASSERT_TRUE(tooling::runToolOnCode(Factory->create(), Code))
   << "Parsing error in \"" << Code << "\"";
   RewriterTestContext Context;
   FileID ID = Context.createInMemoryFile("input.cc", Code);
-  EXPECT_TRUE(tooling::applyAllReplacements(Callback.getReplacements(),
+  EXPECT_TRUE(tooling::applyAllReplacements(FileToReplace["input.cc"],
 Context.Rewrite));
   EXPECT_EQ(Expected, Context.getRewrittenText(ID));
 }
@@ -61,39 +60,64 @@
   std::string Code = "void f() { int i = 1; }";
   std::string Expected = "void f() { int i = 2; }";
   ReplaceStmtWithText Callback("id", "2");
-  expectRewritten(Code, Expected, id("id", expr(integerLiteral())),
-  Callback);
+  expectRewritten(Code, Expected, id("id", expr(integerLiteral())), Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesStmtWithStmt) {
   std::string Code = "void f() { int i = false ? 1 : i * 2; }";
   std::string Expected = "void f() { int i = i * 2; }";
   ReplaceStmtWithStmt Callback("always-false", "should-be");
-  expectRewritten(Code, Expected,
-  id("always-false", conditionalOperator(
-  hasCondition(cxxBoolLiteral(equals(false))),
-  hasFalseExpression(id("should-be", expr(),
+  expectRewritten(
+  Code, Expected,
+  id("always-false",
+ conditionalOperator(hasCondition(cxxBoolLiteral(equals(false))),
+ hasFalseExpression(id("should-be", expr(),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesIfStmt) {
   std::string Code = "bool a; void f() { if (a) f(); else a = true; }";
   std::string Expected = "bool a; void f() { f(); }";
   ReplaceIfStmtWithItsBody Callback("id", true);
-  expectRewritten(Code, Expected,
-  id("id", ifStmt(
-  hasCondition(implicitCastExpr(hasSourceExpression(
-  declRefExpr(to(varDecl(hasName("a"),
+  expectRewritten(
+  Code, Expected,
+  id("id", ifStmt(hasCondition(implicitCastExpr(hasSourceExpression(
+   declRefExpr(to(varDecl(hasName("a"),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, RemovesEntireIfOnEmptyElse) {
   std::string Code = "void f() { if (false) int i = 0; }";
   std::string Expected = "void f() {  }";
   ReplaceIfStmtWithItsBody Callback("id", false);
   expectRewritten(Code, Expected,
-  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
-  Callback);
+  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
+  Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateJustText) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { FOO }";
+  ReplaceNodeWithTemplate Callback("id", "FOO");
+  expectRewritten(Code, Expected, id("id", declStmt()), Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateSimpleSubst) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { long x = 1; }";
+  ReplaceNodeWithTemplate Callback("decl", "long x = ${init}");
+  expectRewritten(Code, Expected,
+  id("decl", varDecl(hasInitializer(id("init", expr(),
+  Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateLiteral) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { string x = \"$-1\"; }";
+  ReplaceNodeWithTemplate Callback("decl", "string x = \"$$-${init}\"");
+  expectRewritten(Code, Expected,
+  id("decl", varDecl(hasInitializer(id("init", 

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert updated this revision to Diff 87574.
jbangert added a comment.

Address code feedback & Reformat with clang-format --style llvm.


https://reviews.llvm.org/D29621

Files:
  include/clang/Tooling/RefactoringCallbacks.h
  lib/Tooling/RefactoringCallbacks.cpp
  unittests/Tooling/RefactoringCallbacksTest.cpp

Index: unittests/Tooling/RefactoringCallbacksTest.cpp
===
--- unittests/Tooling/RefactoringCallbacksTest.cpp
+++ unittests/Tooling/RefactoringCallbacksTest.cpp
@@ -7,31 +7,30 @@
 //
 //===--===//
 
-#include "clang/Tooling/RefactoringCallbacks.h"
 #include "RewriterTestContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/RefactoringCallbacks.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tooling {
 
 using namespace ast_matchers;
 
 template 
-void expectRewritten(const std::string ,
- const std::string ,
- const T ,
- RefactoringCallback ) {
-  MatchFinder Finder;
+void expectRewritten(const std::string , const std::string ,
+ const T , RefactoringCallback ) {
+  std::map FileToReplace;
+  ASTMatchRefactorer Finder(FileToReplace);
   Finder.addMatcher(AMatcher, );
   std::unique_ptr Factory(
   tooling::newFrontendActionFactory());
   ASSERT_TRUE(tooling::runToolOnCode(Factory->create(), Code))
   << "Parsing error in \"" << Code << "\"";
   RewriterTestContext Context;
   FileID ID = Context.createInMemoryFile("input.cc", Code);
-  EXPECT_TRUE(tooling::applyAllReplacements(Callback.getReplacements(),
+  EXPECT_TRUE(tooling::applyAllReplacements(FileToReplace["input.cc"],
 Context.Rewrite));
   EXPECT_EQ(Expected, Context.getRewrittenText(ID));
 }
@@ -61,39 +60,64 @@
   std::string Code = "void f() { int i = 1; }";
   std::string Expected = "void f() { int i = 2; }";
   ReplaceStmtWithText Callback("id", "2");
-  expectRewritten(Code, Expected, id("id", expr(integerLiteral())),
-  Callback);
+  expectRewritten(Code, Expected, id("id", expr(integerLiteral())), Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesStmtWithStmt) {
   std::string Code = "void f() { int i = false ? 1 : i * 2; }";
   std::string Expected = "void f() { int i = i * 2; }";
   ReplaceStmtWithStmt Callback("always-false", "should-be");
-  expectRewritten(Code, Expected,
-  id("always-false", conditionalOperator(
-  hasCondition(cxxBoolLiteral(equals(false))),
-  hasFalseExpression(id("should-be", expr(),
+  expectRewritten(
+  Code, Expected,
+  id("always-false",
+ conditionalOperator(hasCondition(cxxBoolLiteral(equals(false))),
+ hasFalseExpression(id("should-be", expr(),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesIfStmt) {
   std::string Code = "bool a; void f() { if (a) f(); else a = true; }";
   std::string Expected = "bool a; void f() { f(); }";
   ReplaceIfStmtWithItsBody Callback("id", true);
-  expectRewritten(Code, Expected,
-  id("id", ifStmt(
-  hasCondition(implicitCastExpr(hasSourceExpression(
-  declRefExpr(to(varDecl(hasName("a"),
+  expectRewritten(
+  Code, Expected,
+  id("id", ifStmt(hasCondition(implicitCastExpr(hasSourceExpression(
+   declRefExpr(to(varDecl(hasName("a"),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, RemovesEntireIfOnEmptyElse) {
   std::string Code = "void f() { if (false) int i = 0; }";
   std::string Expected = "void f() {  }";
   ReplaceIfStmtWithItsBody Callback("id", false);
   expectRewritten(Code, Expected,
-  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
-  Callback);
+  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
+  Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateJustText) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { FOO }";
+  ReplaceNodeWithTemplate Callback("id", "FOO");
+  expectRewritten(Code, Expected, id("id", declStmt()), Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateSimpleSubst) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { long x = 1; }";
+  ReplaceNodeWithTemplate Callback("decl", "long x = ${init}");
+  expectRewritten(Code, Expected,
+  id("decl", varDecl(hasInitializer(id("init", expr(),
+  Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateLiteral) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { string x = \"$-1\"; }";
+  ReplaceNodeWithTemplate Callback("decl", "string x = \"$$-${init}\"");
+  expectRewritten(Code, Expected,
+  

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: include/clang/Tooling/RefactoringCallbacks.h:61
+MatchFinder.addMatcher(Matcher, Callback);
+Callbacks.emplace_back(Callback);
+  }

Why emplace_back instead of push_back?



Comment at: lib/Tooling/RefactoringCallbacks.cpp:160
+  for (size_t Index = 0; Index < ToTemplate.size();) {
+if (ToTemplate[Index] == '$') {
+  if (ToTemplate.substr(Index, 2) == "$$") {

We should do the parsing in the constructor, instead of on each match.
We could store a vector of {string, bool is_id;} that we just iterate during 
each match.
It also has the advantage of hitting the error+assert before in the run.



Comment at: lib/Tooling/RefactoringCallbacks.cpp:174
+ToTemplate.substr(Index + 2, EndOfIdentifier - Index - 2);
+if (NodeMap.count(SourceNodeName) == 0) {
+  llvm::errs()

This is making two lookups into the map when one is enough.



Comment at: lib/Tooling/RefactoringCallbacks.cpp:192
+  size_t NextIndex = ToTemplate.find('$', Index + 1);
+  ToText = ToText + ToTemplate.substr(Index, NextIndex - Index);
+  Index = NextIndex;

X += instead of X = X +


https://reviews.llvm.org/D29621



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: lib/Tooling/RefactoringCallbacks.cpp:42
+  void HandleTranslationUnit(ASTContext ) override {
+for (const auto  : Refactoring.Callbacks) {
+  Callback->getReplacements().clear();

Could you add a comment here explaining why the replacements in callbacks need 
to be cleared?



Comment at: lib/Tooling/RefactoringCallbacks.cpp:165
+continue;
+  } else if (ToTemplate.substr(Index, 2) == "${") {
+size_t EndOfIdentifier = ToTemplate.find("}", Index);

Just use `if`  since there is a `continue`?



Comment at: lib/Tooling/RefactoringCallbacks.cpp:170
+   << ToTemplate.substr(Index) << "\n";
+  assert(false);
+}

You might want to use `llvm_unreachable` instead so that it also bails out in 
non-assertion build.



Comment at: lib/Tooling/RefactoringCallbacks.cpp:211
+
+}  // end namespace tooling
+}  // end namespace clang

Looks like you are using clang-format with Google-style. Please reformat your 
changes with LLVM style.


https://reviews.llvm.org/D29621



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

[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert created this revision.

This is the first change as part of developing a clang-query based search and 
replace tool.


https://reviews.llvm.org/D29621

Files:
  include/clang/Tooling/RefactoringCallbacks.h
  lib/Tooling/RefactoringCallbacks.cpp
  unittests/Tooling/RefactoringCallbacksTest.cpp

Index: unittests/Tooling/RefactoringCallbacksTest.cpp
===
--- unittests/Tooling/RefactoringCallbacksTest.cpp
+++ unittests/Tooling/RefactoringCallbacksTest.cpp
@@ -7,31 +7,30 @@
 //
 //===--===//
 
-#include "clang/Tooling/RefactoringCallbacks.h"
 #include "RewriterTestContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/RefactoringCallbacks.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tooling {
 
 using namespace ast_matchers;
 
 template 
-void expectRewritten(const std::string ,
- const std::string ,
- const T ,
- RefactoringCallback ) {
-  MatchFinder Finder;
+void expectRewritten(const std::string , const std::string ,
+ const T , RefactoringCallback ) {
+  std::map FileToReplace;
+  ASTMatchRefactorer Finder(FileToReplace);
   Finder.addMatcher(AMatcher, );
   std::unique_ptr Factory(
   tooling::newFrontendActionFactory());
   ASSERT_TRUE(tooling::runToolOnCode(Factory->create(), Code))
   << "Parsing error in \"" << Code << "\"";
   RewriterTestContext Context;
   FileID ID = Context.createInMemoryFile("input.cc", Code);
-  EXPECT_TRUE(tooling::applyAllReplacements(Callback.getReplacements(),
+  EXPECT_TRUE(tooling::applyAllReplacements(FileToReplace["input.cc"],
 Context.Rewrite));
   EXPECT_EQ(Expected, Context.getRewrittenText(ID));
 }
@@ -61,40 +60,65 @@
   std::string Code = "void f() { int i = 1; }";
   std::string Expected = "void f() { int i = 2; }";
   ReplaceStmtWithText Callback("id", "2");
-  expectRewritten(Code, Expected, id("id", expr(integerLiteral())),
-  Callback);
+  expectRewritten(Code, Expected, id("id", expr(integerLiteral())), Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesStmtWithStmt) {
   std::string Code = "void f() { int i = false ? 1 : i * 2; }";
   std::string Expected = "void f() { int i = i * 2; }";
   ReplaceStmtWithStmt Callback("always-false", "should-be");
-  expectRewritten(Code, Expected,
-  id("always-false", conditionalOperator(
-  hasCondition(cxxBoolLiteral(equals(false))),
-  hasFalseExpression(id("should-be", expr(),
+  expectRewritten(
+  Code, Expected,
+  id("always-false",
+ conditionalOperator(hasCondition(cxxBoolLiteral(equals(false))),
+ hasFalseExpression(id("should-be", expr(),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, ReplacesIfStmt) {
   std::string Code = "bool a; void f() { if (a) f(); else a = true; }";
   std::string Expected = "bool a; void f() { f(); }";
   ReplaceIfStmtWithItsBody Callback("id", true);
-  expectRewritten(Code, Expected,
-  id("id", ifStmt(
-  hasCondition(implicitCastExpr(hasSourceExpression(
-  declRefExpr(to(varDecl(hasName("a"),
+  expectRewritten(
+  Code, Expected,
+  id("id", ifStmt(hasCondition(implicitCastExpr(hasSourceExpression(
+   declRefExpr(to(varDecl(hasName("a"),
   Callback);
 }
 
 TEST(RefactoringCallbacksTest, RemovesEntireIfOnEmptyElse) {
   std::string Code = "void f() { if (false) int i = 0; }";
   std::string Expected = "void f() {  }";
   ReplaceIfStmtWithItsBody Callback("id", false);
   expectRewritten(Code, Expected,
-  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
-  Callback);
+  id("id", ifStmt(hasCondition(cxxBoolLiteral(equals(false),
+  Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateJustText) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { FOO }";
+  ReplaceNodeWithTemplate Callback("id", "FOO");
+  expectRewritten(Code, Expected, id("id", declStmt()), Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateSimpleSubst) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { long x = 1; }";
+  ReplaceNodeWithTemplate Callback("decl", "long x = ${init}");
+  expectRewritten(Code, Expected,
+  id("decl", varDecl(hasInitializer(id("init", expr(),
+  Callback);
+}
+
+TEST(RefactoringCallbacksTest, TemplateLiteral) {
+  std::string Code = "void f() { int i = 1; }";
+  std::string Expected = "void f() { string x = \"$-1\"; }";
+  ReplaceNodeWithTemplate Callback("decl", "string x = \"$$-${init}\"");
+  expectRewritten(Code, Expected,
+  id("decl",