[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357768: [LibTooling] Add Transformer, a library for 
source-to-source transformations. (authored by ymandel, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59376?vs=193869=193881#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376

Files:
  cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h
  cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
  cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
  cfe/trunk/unittests/Tooling/CMakeLists.txt
  cfe/trunk/unittests/Tooling/TransformerTest.cpp

Index: cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
===
--- cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
+++ cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
@@ -13,6 +13,7 @@
   Rename/USRFindingAction.cpp
   Rename/USRLocFinder.cpp
   SourceCode.cpp
+  Transformer.cpp
 
   LINK_LIBS
   clangAST
Index: cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
===
--- cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
+++ cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
@@ -0,0 +1,203 @@
+//===--- Transformer.cpp - Transformer library implementation ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include "clang/AST/Expr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "clang/Tooling/Refactoring/SourceCode.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+#include 
+#include 
+#include 
+#include 
+
+using namespace clang;
+using namespace tooling;
+
+using ast_matchers::MatchFinder;
+using ast_type_traits::ASTNodeKind;
+using ast_type_traits::DynTypedNode;
+using llvm::Error;
+using llvm::Expected;
+using llvm::Optional;
+using llvm::StringError;
+using llvm::StringRef;
+using llvm::Twine;
+
+using MatchResult = MatchFinder::MatchResult;
+
+// Did the text at this location originate in a macro definition (aka. body)?
+// For example,
+//
+//   #define NESTED(x) x
+//   #define MACRO(y) { int y  = NESTED(3); }
+//   if (true) MACRO(foo)
+//
+// The if statement expands to
+//
+//   if (true) { int foo = 3; }
+//   ^ ^
+//   Loc1  Loc2
+//
+// For SourceManager SM, SM.isMacroArgExpansion(Loc1) and
+// SM.isMacroArgExpansion(Loc2) are both true, but isOriginMacroBody(sm, Loc1)
+// is false, because "foo" originated in the source file (as an argument to a
+// macro), whereas isOriginMacroBody(SM, Loc2) is true, because "3" originated
+// in the definition of MACRO.
+static bool isOriginMacroBody(const clang::SourceManager ,
+  clang::SourceLocation Loc) {
+  while (Loc.isMacroID()) {
+if (SM.isMacroBodyExpansion(Loc))
+  return true;
+// Otherwise, it must be in an argument, so we continue searching up the
+// invocation stack. getImmediateMacroCallerLoc() gives the location of the
+// argument text, inside the call text.
+Loc = SM.getImmediateMacroCallerLoc(Loc);
+  }
+  return false;
+}
+
+static llvm::Error invalidArgumentError(Twine Message) {
+  return llvm::make_error(llvm::errc::invalid_argument, Message);
+}
+
+static llvm::Error typeError(StringRef Id, const ASTNodeKind ,
+ Twine Message) {
+  return invalidArgumentError(
+  Message + " (node id=" + Id + " kind=" + Kind.asStringRef() + ")");
+}
+
+static llvm::Error missingPropertyError(StringRef Id, Twine Description,
+StringRef Property) {
+  return invalidArgumentError(Description + " requires property '" + Property +
+  "' (node id=" + Id + ")");
+}
+
+static Expected
+getTargetRange(StringRef Target, const DynTypedNode , ASTNodeKind Kind,
+   NodePart TargetPart, ASTContext ) {
+  switch (TargetPart) {
+  case NodePart::Node: {
+// For non-expression statements, associate any trailing semicolon with the
+// statement text.  However, if the target was intended as an expression (as
+// indicated by its kind) then we do not associate any trailing semicolon
+// with it.  We only associate the exact expression text.
+if (Node.get() != nullptr) 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 193869.
ymandel added a comment.

Rebase onto master


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -0,0 +1,389 @@
+//===- unittest/Tooling/TransformerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ast_matchers::anyOf;
+using ast_matchers::argumentCountIs;
+using ast_matchers::callee;
+using ast_matchers::callExpr;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxRecordDecl;
+using ast_matchers::declRefExpr;
+using ast_matchers::expr;
+using ast_matchers::functionDecl;
+using ast_matchers::hasAnyName;
+using ast_matchers::hasArgument;
+using ast_matchers::hasDeclaration;
+using ast_matchers::hasElse;
+using ast_matchers::hasName;
+using ast_matchers::hasType;
+using ast_matchers::ifStmt;
+using ast_matchers::member;
+using ast_matchers::memberExpr;
+using ast_matchers::namedDecl;
+using ast_matchers::on;
+using ast_matchers::pointsTo;
+using ast_matchers::to;
+using ast_matchers::unless;
+
+using llvm::StringRef;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+string(const char*);
+char* c_str();
+int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+
+static ast_matchers::internal::Matcher
+isOrPointsTo(const clang::ast_matchers::DeclarationMatcher ) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+static std::string format(StringRef Code) {
+  const std::vector Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+ADD_FAILURE() << "Could not format code: "
+  << llvm::toString(Formatted.takeError());
+return std::string();
+  }
+  return *Formatted;
+}
+
+static void compareSnippets(StringRef Expected,
+ const llvm::Optional ) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos)
+Actual.erase(I, HL.size());
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(StringRef S) { FileContents[0].second += S; }
+
+  void addFile(StringRef Filename, StringRef Content) {
+FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional rewrite(StringRef Input) {
+std::string Code = ("#include \"header.h\"\n" + Input).str();
+auto Factory = newFrontendActionFactory();
+if (!runToolOnCodeWithArgs(
+Factory->create(), Code, std::vector(), "input.cc",
+"clang-tool", std::make_shared(),
+FileContents)) {
+  return None;
+}
+auto ChangedCodeOrErr =
+applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+if (auto Err = ChangedCodeOrErr.takeError()) {
+  llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
+   << "\n";
+  return None;
+}
+return *ChangedCodeOrErr;
+  }
+
+  void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+Transformer T(std::move(Rule),
+  [this](const AtomicChange ) { Changes.push_back(C); });
+T.registerMatchers();
+compareSnippets(Expected, rewrite(Input));
+  }
+
+  clang::ast_matchers::MatchFinder MatchFinder;
+  AtomicChanges Changes;
+
+private:
+  FileContentMappings FileContents = {{"header.h", ""}};
+};
+
+class TransformerTest : public ClangRefactoringTestBase {
+protected:
+  TransformerTest() { 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.

One more LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 193750.
ymandel added a comment.

Rebasing to parent D60269  so that diffs show 
correctly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -0,0 +1,389 @@
+//===- unittest/Tooling/TransformerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ast_matchers::anyOf;
+using ast_matchers::argumentCountIs;
+using ast_matchers::callee;
+using ast_matchers::callExpr;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxRecordDecl;
+using ast_matchers::declRefExpr;
+using ast_matchers::expr;
+using ast_matchers::functionDecl;
+using ast_matchers::hasAnyName;
+using ast_matchers::hasArgument;
+using ast_matchers::hasDeclaration;
+using ast_matchers::hasElse;
+using ast_matchers::hasName;
+using ast_matchers::hasType;
+using ast_matchers::ifStmt;
+using ast_matchers::member;
+using ast_matchers::memberExpr;
+using ast_matchers::namedDecl;
+using ast_matchers::on;
+using ast_matchers::pointsTo;
+using ast_matchers::to;
+using ast_matchers::unless;
+
+using llvm::StringRef;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+string(const char*);
+char* c_str();
+int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+
+static ast_matchers::internal::Matcher
+isOrPointsTo(const clang::ast_matchers::DeclarationMatcher ) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+static std::string format(StringRef Code) {
+  const std::vector Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+ADD_FAILURE() << "Could not format code: "
+  << llvm::toString(Formatted.takeError());
+return std::string();
+  }
+  return *Formatted;
+}
+
+static void compareSnippets(StringRef Expected,
+ const llvm::Optional ) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos)
+Actual.erase(I, HL.size());
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(StringRef S) { FileContents[0].second += S; }
+
+  void addFile(StringRef Filename, StringRef Content) {
+FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional rewrite(StringRef Input) {
+std::string Code = ("#include \"header.h\"\n" + Input).str();
+auto Factory = newFrontendActionFactory();
+if (!runToolOnCodeWithArgs(
+Factory->create(), Code, std::vector(), "input.cc",
+"clang-tool", std::make_shared(),
+FileContents)) {
+  return None;
+}
+auto ChangedCodeOrErr =
+applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+if (auto Err = ChangedCodeOrErr.takeError()) {
+  llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
+   << "\n";
+  return None;
+}
+return *ChangedCodeOrErr;
+  }
+
+  void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+Transformer T(std::move(Rule),
+  [this](const AtomicChange ) { Changes.push_back(C); });
+T.registerMatchers();
+compareSnippets(Expected, rewrite(Input));
+  }
+
+  clang::ast_matchers::MatchFinder MatchFinder;
+  AtomicChanges Changes;
+
+private:
+  FileContentMappings FileContents = {{"header.h", ""}};
+};
+
+class TransformerTest : public 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 193748.
ymandel added a comment.

Switch from using FixIt to SourceCode, as solution to circular dependency 
problem.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376

Files:
  clang/include/clang/Tooling/FixIt.h
  clang/include/clang/Tooling/Refactoring/SourceCode.h
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/FixIt.cpp
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/SourceCode.cpp
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/FixItTest.cpp
  clang/unittests/Tooling/SourceCodeTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -0,0 +1,389 @@
+//===- unittest/Tooling/TransformerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ast_matchers::anyOf;
+using ast_matchers::argumentCountIs;
+using ast_matchers::callee;
+using ast_matchers::callExpr;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxRecordDecl;
+using ast_matchers::declRefExpr;
+using ast_matchers::expr;
+using ast_matchers::functionDecl;
+using ast_matchers::hasAnyName;
+using ast_matchers::hasArgument;
+using ast_matchers::hasDeclaration;
+using ast_matchers::hasElse;
+using ast_matchers::hasName;
+using ast_matchers::hasType;
+using ast_matchers::ifStmt;
+using ast_matchers::member;
+using ast_matchers::memberExpr;
+using ast_matchers::namedDecl;
+using ast_matchers::on;
+using ast_matchers::pointsTo;
+using ast_matchers::to;
+using ast_matchers::unless;
+
+using llvm::StringRef;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+string(const char*);
+char* c_str();
+int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+
+static ast_matchers::internal::Matcher
+isOrPointsTo(const clang::ast_matchers::DeclarationMatcher ) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+static std::string format(StringRef Code) {
+  const std::vector Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+ADD_FAILURE() << "Could not format code: "
+  << llvm::toString(Formatted.takeError());
+return std::string();
+  }
+  return *Formatted;
+}
+
+static void compareSnippets(StringRef Expected,
+ const llvm::Optional ) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos)
+Actual.erase(I, HL.size());
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(StringRef S) { FileContents[0].second += S; }
+
+  void addFile(StringRef Filename, StringRef Content) {
+FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional rewrite(StringRef Input) {
+std::string Code = ("#include \"header.h\"\n" + Input).str();
+auto Factory = newFrontendActionFactory();
+if (!runToolOnCodeWithArgs(
+Factory->create(), Code, std::vector(), "input.cc",
+"clang-tool", std::make_shared(),
+FileContents)) {
+  return None;
+}
+auto ChangedCodeOrErr =
+applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+if (auto Err = ChangedCodeOrErr.takeError()) {
+  llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
+   << "\n";
+  return None;
+}
+return *ChangedCodeOrErr;
+  }
+
+  void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+Transformer T(std::move(Rule),
+  [this](const AtomicChange ) { Changes.push_back(C); });
+T.registerMatchers();
+

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In D59376#1454834 , @ymandel wrote:

> In D59376#1454768 , @ilya-biryukov 
> wrote:
>
> > Per @ioeric's suggestion: why not move the helper into 
> > `Tooling/Refactoring/ExtendedRange.h`?
> >  If it's in `ToolingRefactoring`, both stencil and transformer can access 
> > it.
> >
> > For external users, a dependency on either `ToolingCore` or 
> > `ToolingRefactoring` should be fine, since they're fine with a dependency 
> > on `Tooling` already.
>
>
> This sounds perfect. Can I do the same for the future additions -- small, 
> focused libraries for each group of functions? I just want to be sure that we 
> don't regret the name "ExtendedRange" when I need to add the next batch.


In clangd, we have a library called `SourceCode.h` that keeps source code 
related helpers including those that deal with ranges. I think it's reasonable 
to use SourceCode.h or something similar here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D59376#1454768 , @ilya-biryukov 
wrote:

> Per @ioeric's suggestion: why not move the helper into 
> `Tooling/Refactoring/ExtendedRange.h`?
>  If it's in `ToolingRefactoring`, both stencil and transformer can access it.
>
> For external users, a dependency on either `ToolingCore` or 
> `ToolingRefactoring` should be fine, since they're fine with a dependency on 
> `Tooling` already.


This sounds perfect. Can I do the same for the future additions -- small, 
focused libraries for each group of functions? I just want to be sure that we 
don't regret the name "ExtendedRange" when I need to add the next batch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a subscriber: ioeric.
ilya-biryukov added a comment.

Per @ioeric's suggestion: why not move the helper into 
`Tooling/Refactoring/ExtendedRange.h`?
If it's in `ToolingRefactoring`, both stencil and transformer can access it.

For external users, a dependency on either `ToolingCore` or 
`ToolingRefactoring` should be fine, since they're fine with a dependency on 
`Tooling` already.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D59376#1454748 , @ymandel wrote:

> I propose that I create a new library in Core with `getExtendedRange()` and 
> remove it from FixIt. The other  utility functions that I need will also go 
> there.  We can separately investigate moving the remaining pieces of FixIt 
> into Core.


Moving this to `Core` makes sense to me, the only concern that I have is that 
it's actually a small and focused library as it stands today and adding utility 
functions to it would seem to cause some chaos.
But it's fine as long as the maintainers of the library are ok with this. I'll 
ask around to find out who's actually responsible for the library and get back 
to you...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D59376#1454617 , @ilya-biryukov 
wrote:

> Keeping it **only** in the cpp file also LG if we don't plan to have other 
> usages for now, but please remove the version from `FixIt.h`


The Stencil library will need this as well and I've heard from another user 
who's already using this (a standalone tool, not in clang), so it seems to have 
a general value.  Might the fixit library belong in Core?  I have a whole bunch 
more utility functions coming along, so we need a place for them as well.

I propose that I create a new library in Core with `getExtendedRange()` and 
remove it from FixIt. The other  utility functions that I need will also go 
there.  We can separately investigate moving the remaining pieces of FixIt into 
Core.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Keeping it **only** in the cpp file also LG if we don't plan to have other 
usages for now, but please remove the version from `FixIt.h`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Given that we don't any usages of `getExtendedRange`, could you remove it from 
the `Fixit.h` and move to a header somewhere in the `clangToolingRefactoring` 
(e.g. into `Transformer.h` itself)?

Having two copies of this functions in the codebase does not look good, 
especially given that one of them is never used anyway.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 193584.
ymandel added a subscriber: ABataev.
ymandel added a comment.

Sever dependency of clangToolingRefactor on clangTooling via FixIt.

Transformer used FixIt, which causes a problematic dependency.  This diff copies
the (minimal) code from FixIt to Transformer.cpp to break the dependency. For
the future, though, we should consider whether the FixIt library should live
somewhere accessible to components in Refactoring (perhaps Core?).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -0,0 +1,389 @@
+//===- unittest/Tooling/TransformerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ast_matchers::anyOf;
+using ast_matchers::argumentCountIs;
+using ast_matchers::callee;
+using ast_matchers::callExpr;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxRecordDecl;
+using ast_matchers::declRefExpr;
+using ast_matchers::expr;
+using ast_matchers::functionDecl;
+using ast_matchers::hasAnyName;
+using ast_matchers::hasArgument;
+using ast_matchers::hasDeclaration;
+using ast_matchers::hasElse;
+using ast_matchers::hasName;
+using ast_matchers::hasType;
+using ast_matchers::ifStmt;
+using ast_matchers::member;
+using ast_matchers::memberExpr;
+using ast_matchers::namedDecl;
+using ast_matchers::on;
+using ast_matchers::pointsTo;
+using ast_matchers::to;
+using ast_matchers::unless;
+
+using llvm::StringRef;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+string(const char*);
+char* c_str();
+int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+
+static ast_matchers::internal::Matcher
+isOrPointsTo(const clang::ast_matchers::DeclarationMatcher ) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+static std::string format(StringRef Code) {
+  const std::vector Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+ADD_FAILURE() << "Could not format code: "
+  << llvm::toString(Formatted.takeError());
+return std::string();
+  }
+  return *Formatted;
+}
+
+static void compareSnippets(StringRef Expected,
+ const llvm::Optional ) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos)
+Actual.erase(I, HL.size());
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(StringRef S) { FileContents[0].second += S; }
+
+  void addFile(StringRef Filename, StringRef Content) {
+FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional rewrite(StringRef Input) {
+std::string Code = ("#include \"header.h\"\n" + Input).str();
+auto Factory = newFrontendActionFactory();
+if (!runToolOnCodeWithArgs(
+Factory->create(), Code, std::vector(), "input.cc",
+"clang-tool", std::make_shared(),
+FileContents)) {
+  return None;
+}
+auto ChangedCodeOrErr =
+applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+if (auto Err = ChangedCodeOrErr.takeError()) {
+  llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
+   << "\n";
+  return None;
+}
+return *ChangedCodeOrErr;
+  }
+
+  void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+Transformer T(std::move(Rule),
+  [this](const 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Ilya, this diff fixes the breakage. PTAL.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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

Reopening to fix the breakage caused by previous attempt at commit.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

Re: [PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via cfe-commits]

Thanks.  Any idea why AtomicChange.cpp's inclusion of
clang/Tooling/ReplacementsYaml.h
doesn't cause the same problem?
https://github.com/llvm/llvm-project/blob/master/clang/lib/Tooling/Refactoring/AtomicChange.cpp

On Wed, Apr 3, 2019 at 2:11 PM Alexey Bataev  wrote:

> The problem is that libToolingRefactor is a part of libTooling. And you
> have the dependency from the Tooling/Refactor subdirectory to the outer one
> Tooling/ directory. And it means that libToolingRefactor is a part of
> libTooling, but it must depend on libTooling itself.
>
> -
> Best regards,
> Alexey Bataev
>
> 03.04.2019 14:08, Yitzhak Mandelbaum пишет:
>
> Alexey, thanks for reverting the change. Can you expand on why a
> dependency from libToolingRefactor to libTooling causes a cycle in the dep
> graph? In particular, I can't find the reverse dependency libTooling ->
> libToolingRefactor. If you can expand on that (I presume its a chain rather
> than a direct dependency?) that would be really helpful.
>
> thanks!
>
> On Wed, Apr 3, 2019 at 1:29 PM Yitzhak Mandelbaum 
> wrote:
>
>> I'll revert the change.
>>
>> On Wed, Apr 3, 2019 at 1:00 PM Yitzhak Mandelbaum 
>> wrote:
>>
>>> https://reviews.llvm.org/D60213 for the record
>>>
>>> On Wed, Apr 3, 2019 at 12:55 PM Yitzhak Mandelbaum 
>>> wrote:
>>>
 Thanks.  Do you have build command I can run before/after to verify my
 fix before I submit?

 On Wed, Apr 3, 2019 at 12:43 PM Alexey Bataev via Phabricator <
 revi...@reviews.llvm.org> wrote:

> ABataev added a comment.
>
> Patch breaks the build with the shared libraries, for example,
> http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/9498.
> Seems to me, you need to add an extra dependency on clangTooling.
> clangToolingCore is not enough.
>
>
> Repository:
>   rC Clang
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D59376/new/
>
> https://reviews.llvm.org/D59376
>
>
>
>


smime.p7s
Description: S/MIME Cryptographic Signature
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via cfe-commits]

Alexey, thanks for reverting the change. Can you expand on why a dependency
from libToolingRefactor to libTooling causes a cycle in the dep graph? In
particular, I can't find the reverse dependency libTooling ->
libToolingRefactor. If you can expand on that (I presume its a chain rather
than a direct dependency?) that would be really helpful.

thanks!

On Wed, Apr 3, 2019 at 1:29 PM Yitzhak Mandelbaum 
wrote:

> I'll revert the change.
>
> On Wed, Apr 3, 2019 at 1:00 PM Yitzhak Mandelbaum 
> wrote:
>
>> https://reviews.llvm.org/D60213 for the record
>>
>> On Wed, Apr 3, 2019 at 12:55 PM Yitzhak Mandelbaum 
>> wrote:
>>
>>> Thanks.  Do you have build command I can run before/after to verify my
>>> fix before I submit?
>>>
>>> On Wed, Apr 3, 2019 at 12:43 PM Alexey Bataev via Phabricator <
>>> revi...@reviews.llvm.org> wrote:
>>>
 ABataev added a comment.

 Patch breaks the build with the shared libraries, for example,
 http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/9498.
 Seems to me, you need to add an extra dependency on clangTooling.
 clangToolingCore is not enough.


 Repository:
   rC Clang

 CHANGES SINCE LAST ACTION
   https://reviews.llvm.org/D59376/new/

 https://reviews.llvm.org/D59376






smime.p7s
Description: S/MIME Cryptographic Signature
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via cfe-commits]

I'll revert the change.

On Wed, Apr 3, 2019 at 1:00 PM Yitzhak Mandelbaum 
wrote:

> https://reviews.llvm.org/D60213 for the record
>
> On Wed, Apr 3, 2019 at 12:55 PM Yitzhak Mandelbaum 
> wrote:
>
>> Thanks.  Do you have build command I can run before/after to verify my
>> fix before I submit?
>>
>> On Wed, Apr 3, 2019 at 12:43 PM Alexey Bataev via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> ABataev added a comment.
>>>
>>> Patch breaks the build with the shared libraries, for example,
>>> http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/9498.
>>> Seems to me, you need to add an extra dependency on clangTooling.
>>> clangToolingCore is not enough.
>>>
>>>
>>> Repository:
>>>   rC Clang
>>>
>>> CHANGES SINCE LAST ACTION
>>>   https://reviews.llvm.org/D59376/new/
>>>
>>> https://reviews.llvm.org/D59376
>>>
>>>
>>>
>>>


smime.p7s
Description: S/MIME Cryptographic Signature
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via cfe-commits]

https://reviews.llvm.org/D60213 for the record

On Wed, Apr 3, 2019 at 12:55 PM Yitzhak Mandelbaum 
wrote:

> Thanks.  Do you have build command I can run before/after to verify my fix
> before I submit?
>
> On Wed, Apr 3, 2019 at 12:43 PM Alexey Bataev via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> ABataev added a comment.
>>
>> Patch breaks the build with the shared libraries, for example,
>> http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/9498.
>> Seems to me, you need to add an extra dependency on clangTooling.
>> clangToolingCore is not enough.
>>
>>
>> Repository:
>>   rC Clang
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D59376/new/
>>
>> https://reviews.llvm.org/D59376
>>
>>
>>
>>


smime.p7s
Description: S/MIME Cryptographic Signature
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

You need to provide -DBUILD_SHARED_LIBS=true to cmake. Unfortunately, there is 
a dependency from the libToolingRefactor to libTooling. I don't think it will 
be easy to fix this.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

Re: [PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via cfe-commits]

Thanks.  Do you have build command I can run before/after to verify my fix
before I submit?

On Wed, Apr 3, 2019 at 12:43 PM Alexey Bataev via Phabricator <
revi...@reviews.llvm.org> wrote:

> ABataev added a comment.
>
> Patch breaks the build with the shared libraries, for example,
> http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/9498.
> Seems to me, you need to add an extra dependency on clangTooling.
> clangToolingCore is not enough.
>
>
> Repository:
>   rC Clang
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D59376/new/
>
> https://reviews.llvm.org/D59376
>
>
>
>


smime.p7s
Description: S/MIME Cryptographic Signature
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Patch breaks the build with the shared libraries, for example, 
http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/9498. 
Seems to me, you need to add an extra dependency on clangTooling. 
clangToolingCore is not enough.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC357576: [LibTooling] Add Transformer, a library for 
source-to-source transformations. (authored by ymandel, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59376?vs=193306=193480#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376

Files:
  include/clang/Tooling/Refactoring/Transformer.h
  lib/Tooling/Refactoring/CMakeLists.txt
  lib/Tooling/Refactoring/Transformer.cpp
  unittests/Tooling/CMakeLists.txt
  unittests/Tooling/TransformerTest.cpp

Index: lib/Tooling/Refactoring/Transformer.cpp
===
--- lib/Tooling/Refactoring/Transformer.cpp
+++ lib/Tooling/Refactoring/Transformer.cpp
@@ -0,0 +1,204 @@
+//===--- Transformer.cpp - Transformer library implementation ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+#include "clang/AST/Expr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/FixIt.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+#include 
+#include 
+#include 
+#include 
+
+using namespace clang;
+using namespace tooling;
+
+using ast_matchers::MatchFinder;
+using ast_type_traits::ASTNodeKind;
+using ast_type_traits::DynTypedNode;
+using llvm::Error;
+using llvm::Expected;
+using llvm::Optional;
+using llvm::StringError;
+using llvm::StringRef;
+using llvm::Twine;
+
+using MatchResult = MatchFinder::MatchResult;
+
+// Did the text at this location originate in a macro definition (aka. body)?
+// For example,
+//
+//   #define NESTED(x) x
+//   #define MACRO(y) { int y  = NESTED(3); }
+//   if (true) MACRO(foo)
+//
+// The if statement expands to
+//
+//   if (true) { int foo = 3; }
+//   ^ ^
+//   Loc1  Loc2
+//
+// For SourceManager SM, SM.isMacroArgExpansion(Loc1) and
+// SM.isMacroArgExpansion(Loc2) are both true, but isOriginMacroBody(sm, Loc1)
+// is false, because "foo" originated in the source file (as an argument to a
+// macro), whereas isOriginMacroBody(SM, Loc2) is true, because "3" originated
+// in the definition of MACRO.
+static bool isOriginMacroBody(const clang::SourceManager ,
+  clang::SourceLocation Loc) {
+  while (Loc.isMacroID()) {
+if (SM.isMacroBodyExpansion(Loc))
+  return true;
+// Otherwise, it must be in an argument, so we continue searching up the
+// invocation stack. getImmediateMacroCallerLoc() gives the location of the
+// argument text, inside the call text.
+Loc = SM.getImmediateMacroCallerLoc(Loc);
+  }
+  return false;
+}
+
+static llvm::Error invalidArgumentError(Twine Message) {
+  return llvm::make_error(llvm::errc::invalid_argument, Message);
+}
+
+static llvm::Error typeError(StringRef Id, const ASTNodeKind ,
+ Twine Message) {
+  return invalidArgumentError(
+  Message + " (node id=" + Id + " kind=" + Kind.asStringRef() + ")");
+}
+
+static llvm::Error missingPropertyError(StringRef Id, Twine Description,
+StringRef Property) {
+  return invalidArgumentError(Description + " requires property '" + Property +
+  "' (node id=" + Id + ")");
+}
+
+static Expected
+getTargetRange(StringRef Target, const DynTypedNode , ASTNodeKind Kind,
+   NodePart TargetPart, ASTContext ) {
+  switch (TargetPart) {
+  case NodePart::Node: {
+// For non-expression statements, associate any trailing semicolon with the
+// statement text.  However, if the target was intended as an expression (as
+// indicated by its kind) then we do not associate any trailing semicolon
+// with it.  We only associate the exact expression text.
+if (Node.get() != nullptr) {
+  auto ExprKind = ASTNodeKind::getFromNodeKind();
+  if (!ExprKind.isBaseOf(Kind))
+return fixit::getExtendedRange(Node, tok::TokenKind::semi, Context);
+}
+return CharSourceRange::getTokenRange(Node.getSourceRange());
+  }
+  case NodePart::Member:
+if (auto *M = Node.get())
+  return CharSourceRange::getTokenRange(
+  M->getMemberNameInfo().getSourceRange());
+return typeError(Target, Node.getNodeKind(),
+ "NodePart::Member 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.

LGTM with the new changes. Specifying the `clang::Expr` type explicitly when 
calling `change` looks a bit less clear than the original approach, but also 
looks pretty clear to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

I've removed the dependency on NodeIds given our (off thread) discussions 
regarding their overall value and appropriateness in clang::tooling vs 
clang::ast_matchers. PTAL.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 193306.
ymandel added a comment.

Remove dependency on NodeIds.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -0,0 +1,389 @@
+//===- unittest/Tooling/TransformerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ast_matchers::anyOf;
+using ast_matchers::argumentCountIs;
+using ast_matchers::callee;
+using ast_matchers::callExpr;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxRecordDecl;
+using ast_matchers::declRefExpr;
+using ast_matchers::expr;
+using ast_matchers::functionDecl;
+using ast_matchers::hasAnyName;
+using ast_matchers::hasArgument;
+using ast_matchers::hasDeclaration;
+using ast_matchers::hasElse;
+using ast_matchers::hasName;
+using ast_matchers::hasType;
+using ast_matchers::ifStmt;
+using ast_matchers::member;
+using ast_matchers::memberExpr;
+using ast_matchers::namedDecl;
+using ast_matchers::on;
+using ast_matchers::pointsTo;
+using ast_matchers::to;
+using ast_matchers::unless;
+
+using llvm::StringRef;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+string(const char*);
+char* c_str();
+int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+
+static ast_matchers::internal::Matcher
+isOrPointsTo(const clang::ast_matchers::DeclarationMatcher ) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+static std::string format(StringRef Code) {
+  const std::vector Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+ADD_FAILURE() << "Could not format code: "
+  << llvm::toString(Formatted.takeError());
+return std::string();
+  }
+  return *Formatted;
+}
+
+static void compareSnippets(StringRef Expected,
+ const llvm::Optional ) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos)
+Actual.erase(I, HL.size());
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(StringRef S) { FileContents[0].second += S; }
+
+  void addFile(StringRef Filename, StringRef Content) {
+FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional rewrite(StringRef Input) {
+std::string Code = ("#include \"header.h\"\n" + Input).str();
+auto Factory = newFrontendActionFactory();
+if (!runToolOnCodeWithArgs(
+Factory->create(), Code, std::vector(), "input.cc",
+"clang-tool", std::make_shared(),
+FileContents)) {
+  return None;
+}
+auto ChangedCodeOrErr =
+applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+if (auto Err = ChangedCodeOrErr.takeError()) {
+  llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
+   << "\n";
+  return None;
+}
+return *ChangedCodeOrErr;
+  }
+
+  void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+Transformer T(std::move(Rule),
+  [this](const AtomicChange ) { Changes.push_back(C); });
+T.registerMatchers();
+compareSnippets(Expected, rewrite(Input));
+  }
+
+  clang::ast_matchers::MatchFinder MatchFinder;
+  AtomicChanges Changes;
+
+private:
+  FileContentMappings FileContents = {{"header.h", ""}};
+};
+
+class TransformerTest : public ClangRefactoringTestBase {
+protected:
+  TransformerTest() { 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Thank you for the extremely helpful and detailed review!!




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:110
+  // constructed with the builder class.
+  static constexpr char RootId[] = "___root___";
+};

ilya-biryukov wrote:
> NIT: `llvm::StringLiteral` is a vocabulary type with compile-time size that 
> could be used here.
> Although I don't think it has any actual benefits over char array, so leaving 
> as is also looks good.
Went with StringLiteral -- given that we always wrap RootId in a stringref to 
use it, seems better just to define it that way to begin with.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 193260.
ymandel marked 3 inline comments as done.
ymandel added a comment.

Small tweaks in response to comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -0,0 +1,388 @@
+//===- unittest/Tooling/TransformerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ast_matchers::anyOf;
+using ast_matchers::argumentCountIs;
+using ast_matchers::callee;
+using ast_matchers::callExpr;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxRecordDecl;
+using ast_matchers::declRefExpr;
+using ast_matchers::expr;
+using ast_matchers::functionDecl;
+using ast_matchers::hasAnyName;
+using ast_matchers::hasArgument;
+using ast_matchers::hasDeclaration;
+using ast_matchers::hasElse;
+using ast_matchers::hasName;
+using ast_matchers::hasType;
+using ast_matchers::ifStmt;
+using ast_matchers::member;
+using ast_matchers::memberExpr;
+using ast_matchers::namedDecl;
+using ast_matchers::on;
+using ast_matchers::pointsTo;
+using ast_matchers::to;
+using ast_matchers::unless;
+
+using llvm::StringRef;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+string(const char*);
+char* c_str();
+int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+
+static ast_matchers::internal::Matcher
+isOrPointsTo(const clang::ast_matchers::DeclarationMatcher ) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+static std::string format(StringRef Code) {
+  const std::vector Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+ADD_FAILURE() << "Could not format code: "
+  << llvm::toString(Formatted.takeError());
+return std::string();
+  }
+  return *Formatted;
+}
+
+static void compareSnippets(StringRef Expected,
+ const llvm::Optional ) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos)
+Actual.erase(I, HL.size());
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(StringRef S) { FileContents[0].second += S; }
+
+  void addFile(StringRef Filename, StringRef Content) {
+FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional rewrite(StringRef Input) {
+std::string Code = ("#include \"header.h\"\n" + Input).str();
+auto Factory = newFrontendActionFactory();
+if (!runToolOnCodeWithArgs(
+Factory->create(), Code, std::vector(), "input.cc",
+"clang-tool", std::make_shared(),
+FileContents)) {
+  return None;
+}
+auto ChangedCodeOrErr =
+applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+if (auto Err = ChangedCodeOrErr.takeError()) {
+  llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
+   << "\n";
+  return None;
+}
+return *ChangedCodeOrErr;
+  }
+
+  void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+Transformer T(std::move(Rule),
+  [this](const AtomicChange ) { Changes.push_back(C); });
+T.registerMatchers();
+compareSnippets(Expected, rewrite(Input));
+  }
+
+  clang::ast_matchers::MatchFinder MatchFinder;
+  AtomicChanges Changes;
+
+private:
+  FileContentMappings FileContents = {{"header.h", ""}};
+};
+
+class TransformerTest : public ClangRefactoringTestBase {

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

Looks great, thanks!




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:110
+  // constructed with the builder class.
+  static constexpr char RootId[] = "___root___";
+};

NIT: `llvm::StringLiteral` is a vocabulary type with compile-time size that 
could be used here.
Although I don't think it has any actual benefits over char array, so leaving 
as is also looks good.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:113
+
+/// A fluent, builder class for \c RewriteRule.  See comments on \c 
RewriteRule,
+/// above.

NIT: comma seems redundant, this should probably be: `A fluent builder class`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:70
+//
+// * Explanation: explanation of the rewrite.
+//

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > NIT: maybe mention what it should be used for? we plan to show to to the 
> > > user (e.g. in the clang-tidy fix description), right?
> > Done. But, given that we don't use this field yet, I'm fine deleting it 
> > until a future revision. I'm also fine leaving it given that we know we'll 
> > be needing it later.
> Up to you, to me it feels like the presence of this field defines what this 
> class is used for.
> 1. If there's an explanation, it feels like it should represent a complete 
> fix or refactoring that could be presented to the user.
> 2. Without an explanation, it might feel like something lower-level (e.g. one 
> could write a bunch of RewriteRule whose changes are later combined and 
> surfaced to the user as a full refactoring).
> 
> Both approaches make sense, and I assume (1) is the desired abstraction this 
> class represents, so keeping the field looks ok.
Agreed. Keeping it.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+// \code
+//   RewriteRule R = buildRule(functionDecl(...)).replaceWith(...);
+// \endcode

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > Could you also add examples on how to apply the rewrite rule here?
> > > So that the users can have an idea about the full workflow when reading 
> > > the code.
> > Is this what you had in mind? Unlike clang-tidy, there is no neat 
> > infrastructure into which we can drop it.
> Yeah, exactly, but could we keep is a bit shorter by removing the pieces 
> which are non-relevant to the actual transformer usage.
> Something like:
> ```
> // To apply a rule to the clang AST, use Transformer class:
> // \code
> //   AtomicChanges Changes;
> //   Transformer T(
> //   MyRule, [](const AtomicChange ) { Changes.push_back(C); 
> };);
> //   ast_matchers::MatchFinder MatchFinder;
> //   T.registerMatchers();
> //   // ... insert code to run the ast matchers.
> //   // Consume the changes.
> //   applyAtomicChanges(..., Changes);
> ```
> 
> Or just mention that `Transformer` class should be used to apply the rewrite 
> rule and obtain the corresponding replacements.
went w/ the second suggestion.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:136
+
+  RewriteRule(ast_matchers::internal::DynTypedMatcher M)
+  : Matcher(initMatcher(M)), Target(RootId), 
TargetKind(M.getSupportedKind()),

ilya-biryukov wrote:
> NIT: Maybe remove the constructor and let the builder handle this?
> Technically, users can break the invariants after creating the rewrite rules 
> anyway, so having this constructor does not buy much in terms of safer coding 
> practices, but makes it harder to use `RewriteRule` as a plain struct 
> (specifically, having no default constructor might make it awkward).
> 
> Up to you, of course.
Agreed, but DynTypedMatcher has no default constructor, so we have to provide 
something.  I dropped the constructor, but added an initializer to `Matcher` to 
enable the default constructor.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:184
+RewriteRuleBuilder buildRule(ast_matchers::internal::Matcher M) {
+  return RewriteRuleBuilder(M);
+}

ilya-biryukov wrote:
> Isn't this overload redundant in presence of `buildRule(DynTypedMatcher)`? 
> Both seem to call into the constructor that accept a `DynTypedMatcher`, so 
> `Matcher` is convertible to `DynTypedMatcher`, right?.
I think I was avoiding some extra construction, but even if so, I can't see its 
worth the added complexity. thx



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:133
+
+namespace clang {
+namespace tooling {

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > NIT: remove namespaces for consistency with the rest of the code.
> > > 
> > > (Probably a leftover from the previous version)
> > This is necessary as far as I can tell. W/o it, I get a linker failure 
> > (missing definition). Based on the declaration in the header, the compiler 
> > resolves the reference below to clang::tooling::applyRewriteRule() but this 
> > definition ends up in the global namespace.
> > 
> > I think the using decls only work for method definitions -- the type seems 
> > to resolve to be the one in the namespace. But free functions don't get the 
> > same treatment. Unless I"m doing something wrong?
> Yeah, you should explicitly qualify to let the compiler know the namespace of 
> a corresponding declaration:
> ```
> Expected
> clang::tooling::applyRewriteRule(...) {
>   // ...
> }
> ```
> 
> `tooling::applyRewriteRule` would also work since we have `using namespace 
> clang::tooling;` at 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 193219.
ymandel marked 28 inline comments as done.
ymandel added a comment.

Assorted changes in response to comments.

Most notably, dropped constructor from RewriteRule, in favor of putting 
meaningful initialization in the builder.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -0,0 +1,388 @@
+//===- unittest/Tooling/TransformerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ast_matchers::anyOf;
+using ast_matchers::argumentCountIs;
+using ast_matchers::callee;
+using ast_matchers::callExpr;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxRecordDecl;
+using ast_matchers::declRefExpr;
+using ast_matchers::expr;
+using ast_matchers::functionDecl;
+using ast_matchers::hasAnyName;
+using ast_matchers::hasArgument;
+using ast_matchers::hasDeclaration;
+using ast_matchers::hasElse;
+using ast_matchers::hasName;
+using ast_matchers::hasType;
+using ast_matchers::ifStmt;
+using ast_matchers::member;
+using ast_matchers::memberExpr;
+using ast_matchers::namedDecl;
+using ast_matchers::on;
+using ast_matchers::pointsTo;
+using ast_matchers::to;
+using ast_matchers::unless;
+
+using llvm::StringRef;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+string(const char*);
+char* c_str();
+int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+
+static ast_matchers::internal::Matcher
+isOrPointsTo(const clang::ast_matchers::DeclarationMatcher ) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+static std::string format(StringRef Code) {
+  const std::vector Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+ADD_FAILURE() << "Could not format code: "
+  << llvm::toString(Formatted.takeError());
+return std::string();
+  }
+  return *Formatted;
+}
+
+static void compareSnippets(StringRef Expected,
+ const llvm::Optional ) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos)
+Actual.erase(I, HL.size());
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(StringRef S) { FileContents[0].second += S; }
+
+  void addFile(StringRef Filename, StringRef Content) {
+FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional rewrite(StringRef Input) {
+std::string Code = ("#include \"header.h\"\n" + Input).str();
+auto Factory = newFrontendActionFactory();
+if (!runToolOnCodeWithArgs(
+Factory->create(), Code, std::vector(), "input.cc",
+"clang-tool", std::make_shared(),
+FileContents)) {
+  return None;
+}
+auto ChangedCodeOrErr =
+applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+if (auto Err = ChangedCodeOrErr.takeError()) {
+  llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
+   << "\n";
+  return None;
+}
+return *ChangedCodeOrErr;
+  }
+
+  void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+Transformer T(std::move(Rule),
+  [this](const AtomicChange ) { Changes.push_back(C); });
+T.registerMatchers();
+compareSnippets(Expected, rewrite(Input));
+  }
+
+  clang::ast_matchers::MatchFinder MatchFinder;
+  AtomicChanges Changes;
+
+private:
+  

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

Looks neat, thanks!
A final set of NITs from my side, I don't think any of the comments have to do 
with the actual design of the library, so expect LGTM in the next round.




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:70
+//
+// * Explanation: explanation of the rewrite.
+//

ymandel wrote:
> ilya-biryukov wrote:
> > NIT: maybe mention what it should be used for? we plan to show to to the 
> > user (e.g. in the clang-tidy fix description), right?
> Done. But, given that we don't use this field yet, I'm fine deleting it until 
> a future revision. I'm also fine leaving it given that we know we'll be 
> needing it later.
Up to you, to me it feels like the presence of this field defines what this 
class is used for.
1. If there's an explanation, it feels like it should represent a complete fix 
or refactoring that could be presented to the user.
2. Without an explanation, it might feel like something lower-level (e.g. one 
could write a bunch of RewriteRule whose changes are later combined and 
surfaced to the user as a full refactoring).

Both approaches make sense, and I assume (1) is the desired abstraction this 
class represents, so keeping the field looks ok.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+// \code
+//   RewriteRule R = buildRule(functionDecl(...)).replaceWith(...);
+// \endcode

ymandel wrote:
> ilya-biryukov wrote:
> > Could you also add examples on how to apply the rewrite rule here?
> > So that the users can have an idea about the full workflow when reading the 
> > code.
> Is this what you had in mind? Unlike clang-tidy, there is no neat 
> infrastructure into which we can drop it.
Yeah, exactly, but could we keep is a bit shorter by removing the pieces which 
are non-relevant to the actual transformer usage.
Something like:
```
// To apply a rule to the clang AST, use Transformer class:
// \code
//   AtomicChanges Changes;
//   Transformer T(
//   MyRule, [](const AtomicChange ) { Changes.push_back(C); };);
//   ast_matchers::MatchFinder MatchFinder;
//   T.registerMatchers();
//   // ... insert code to run the ast matchers.
//   // Consume the changes.
//   applyAtomicChanges(..., Changes);
```

Or just mention that `Transformer` class should be used to apply the rewrite 
rule and obtain the corresponding replacements.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:82
+// Parameters can be declared explicitly using the NodeId type and its
+// derivatives or left implicit by using the native support for binding ids in
+// the clang matchers.

NIT: Maybe be more specific: **string-based** binding ids?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:125
+  // Id used as the default target of each match. The node described by the
+  // matcher is guaranteed to be bound this id, for all rewrite rules.
+  static constexpr char RootId[] = "___root___";

NIT: bound **to** this id



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:136
+
+  RewriteRule(ast_matchers::internal::DynTypedMatcher M)
+  : Matcher(initMatcher(M)), Target(RootId), 
TargetKind(M.getSupportedKind()),

NIT: Maybe remove the constructor and let the builder handle this?
Technically, users can break the invariants after creating the rewrite rules 
anyway, so having this constructor does not buy much in terms of safer coding 
practices, but makes it harder to use `RewriteRule` as a plain struct 
(specifically, having no default constructor might make it awkward).

Up to you, of course.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:158
+  template 
+  RewriteRuleBuilder &(const TypedNodeId ,
+  NodePart Part = NodePart::Node) &&;

NIT: return by value to make the signatures less clunky. RVO should insure the 
generated code is the same in practice.
Bonus point: that'd mean we don't need `&&` qualifier on the builder functions 
too, only at `consume()`.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:184
+RewriteRuleBuilder buildRule(ast_matchers::internal::Matcher M) {
+  return RewriteRuleBuilder(M);
+}

Isn't this overload redundant in presence of `buildRule(DynTypedMatcher)`? Both 
seem to call into the constructor that accept a `DynTypedMatcher`, so 
`Matcher` is convertible to `DynTypedMatcher`, right?.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:133
+
+namespace clang {
+namespace tooling {

ymandel wrote:
> ilya-biryukov wrote:
> > NIT: remove namespaces for consistency with the rest of the code.
> > 
> > (Probably a leftover from the previous version)
> This is necessary as far as 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 19 inline comments as done.
ymandel added a comment.

Thanks for (more) helpful comments.  I think the code is a lot better now than 
it started out. :)

Also, converted `RewriteRule` to a simple struct as per our discussion.




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:70
+//
+// * Explanation: explanation of the rewrite.
+//

ilya-biryukov wrote:
> NIT: maybe mention what it should be used for? we plan to show to to the user 
> (e.g. in the clang-tidy fix description), right?
Done. But, given that we don't use this field yet, I'm fine deleting it until a 
future revision. I'm also fine leaving it given that we know we'll be needing 
it later.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+// \code
+//   RewriteRule R = buildRule(functionDecl(...)).replaceWith(...);
+// \endcode

ilya-biryukov wrote:
> Could you also add examples on how to apply the rewrite rule here?
> So that the users can have an idea about the full workflow when reading the 
> code.
Is this what you had in mind? Unlike clang-tidy, there is no neat 
infrastructure into which we can drop it.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:97
+  ast_matchers::internal::DynTypedMatcher Matcher;
+  // The (bound) id of the node whose source will be replaced.  This id should
+  // never be the empty string.

ilya-biryukov wrote:
> NIT: maybe assert this invariant in the constructor?
The constructor sets this field, so no need to assert.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:122
+  // The bound id of the node corresponding to the matcher.
+  static llvm::StringRef matchedNode() { return RootId; }
+

ilya-biryukov wrote:
> This method does not seem to be used anywhere.
> Are we missing the usages in this patch? Maybe remove it from the initial 
> implementation and re-add later when we have the callsites?
It was used above in makeMatcher.  But, it was overkill -- users can just 
reference RootId directly, so I've removed it.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:175
+  RewriteRuleBuilder &(std::string Explanation) && {
+return std::move(std::move(*this).because(text(std::move(Explanation;
+  }

ilya-biryukov wrote:
> NIT: the top-level `std::move` looks redundant, maybe remove it?
Yes, not sure why did that. thanks.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:133
+
+namespace clang {
+namespace tooling {

ilya-biryukov wrote:
> NIT: remove namespaces for consistency with the rest of the code.
> 
> (Probably a leftover from the previous version)
This is necessary as far as I can tell. W/o it, I get a linker failure (missing 
definition). Based on the declaration in the header, the compiler resolves the 
reference below to clang::tooling::applyRewriteRule() but this definition ends 
up in the global namespace.

I think the using decls only work for method definitions -- the type seems to 
resolve to be the one in the namespace. But free functions don't get the same 
treatment. Unless I"m doing something wrong?



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:150
+  llvm::handleErrors(TargetOrErr.takeError(), [](StringError ) {
+return invalidArgumentError("Failure targeting node" +
+Rule.target() + ": " + E.getMessage());

ilya-biryukov wrote:
> NIT: consider simply propagating the original error up the stack in that case 
> to avoid boilerplate.
> Although adding the `.target()` information to the error might be useful, so 
> up to you.
integrated id into getTarget so we can avoid this error handling step.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:157
+  isOriginMacroBody(*Match.SourceManager, Target.getBegin()))
+return Transformation();
+

ilya-biryukov wrote:
> Why not return an error in case of macros too? Is there any use of the empty 
> transformation to the clients? 
> Alternatively, we might want to document this behavior (applyRewriteRule can 
> return empty transformations) and it's rationale.
I think the common case is that we simply want to skip macros -- there's 
nothing erroneous about them, they're just usually hard to deal w/ correctly.  
That said, we might want to change this to *never* return an error and just 
assert when things go wrong. The advantage of the current design is that 
callers can in principle ignore failed rewrites.

However, in practice, if the rewrite fails that means it had a bug, so it might 
be best to asssert().  Only real advantage, then, is that this is easier to 
test since it doesn't crash the program.

WDYT?



Comment at: 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 192889.
ymandel marked an inline comment as done.
ymandel added a comment.

- Assorted changes in response to comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -0,0 +1,388 @@
+//===- unittest/Tooling/TransformerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ast_matchers::anyOf;
+using ast_matchers::argumentCountIs;
+using ast_matchers::callee;
+using ast_matchers::callExpr;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxRecordDecl;
+using ast_matchers::declRefExpr;
+using ast_matchers::expr;
+using ast_matchers::functionDecl;
+using ast_matchers::hasAnyName;
+using ast_matchers::hasArgument;
+using ast_matchers::hasDeclaration;
+using ast_matchers::hasElse;
+using ast_matchers::hasName;
+using ast_matchers::hasType;
+using ast_matchers::ifStmt;
+using ast_matchers::member;
+using ast_matchers::memberExpr;
+using ast_matchers::namedDecl;
+using ast_matchers::on;
+using ast_matchers::pointsTo;
+using ast_matchers::to;
+using ast_matchers::unless;
+
+using llvm::StringRef;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+string(const char*);
+char* c_str();
+int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+
+ast_matchers::internal::Matcher
+isOrPointsTo(const clang::ast_matchers::DeclarationMatcher ) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+std::string format(StringRef Code) {
+  const std::vector Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+ADD_FAILURE() << "Could not format code: "
+  << llvm::toString(Formatted.takeError());
+return std::string();
+  }
+  return *Formatted;
+}
+
+void compareSnippets(StringRef Expected,
+ const llvm::Optional ) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos)
+Actual.erase(I, HL.size());
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(StringRef S) { FileContents[0].second += S; }
+
+  void addFile(StringRef Filename, StringRef Content) {
+FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional rewrite(StringRef Input) {
+std::string Code = ("#include \"header.h\"\n" + Input).str();
+auto Factory = newFrontendActionFactory();
+if (!runToolOnCodeWithArgs(
+Factory->create(), Code, std::vector(), "input.cc",
+"clang-tool", std::make_shared(),
+FileContents)) {
+  return None;
+}
+auto ChangedCodeOrErr =
+applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+if (auto Err = ChangedCodeOrErr.takeError()) {
+  llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
+   << "\n";
+  return None;
+}
+return *ChangedCodeOrErr;
+  }
+
+  void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+Transformer T(std::move(Rule),
+  [this](const AtomicChange ) { Changes.push_back(C); });
+T.registerMatchers();
+compareSnippets(Expected, rewrite(Input));
+  }
+
+  clang::ast_matchers::MatchFinder MatchFinder;
+  AtomicChanges Changes;
+
+private:
+  FileContentMappings FileContents = {{"header.h", ""}};
+};
+
+class TransformerTest : public ClangRefactoringTestBase {
+protected:
+  

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.

The only real question I have is about returning an error vs an empty 
transformation in of macros.
The rest are just NITs.

Thanks for the change! I'll get to the NodeId patch right away :-)




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:170
+
+  RewriteRuleBuilder(const RewriteRuleBuilder &) = default;
+  RewriteRuleBuilder(RewriteRuleBuilder &&) = default;

ymandel wrote:
> ilya-biryukov wrote:
> > NIT: maybe remove the `=default` copy and move ctors and assignments?
> > They should be generated automatically anyway, right?
> Sure. I was going based on google's style recommendations, but personally i 
> prefer leaving them implicit.
+1. Means less boilterplate.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:70
+//
+// * Explanation: explanation of the rewrite.
+//

NIT: maybe mention what it should be used for? we plan to show to to the user 
(e.g. in the clang-tidy fix description), right?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+// \code
+//   RewriteRule R = buildRule(functionDecl(...)).replaceWith(...);
+// \endcode

Could you also add examples on how to apply the rewrite rule here?
So that the users can have an idea about the full workflow when reading the 
code.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:97
+  ast_matchers::internal::DynTypedMatcher Matcher;
+  // The (bound) id of the node whose source will be replaced.  This id should
+  // never be the empty string.

NIT: maybe assert this invariant in the constructor?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:122
+  // The bound id of the node corresponding to the matcher.
+  static llvm::StringRef matchedNode() { return RootId; }
+

This method does not seem to be used anywhere.
Are we missing the usages in this patch? Maybe remove it from the initial 
implementation and re-add later when we have the callsites?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:175
+  RewriteRuleBuilder &(std::string Explanation) && {
+return std::move(std::move(*this).because(text(std::move(Explanation;
+  }

NIT: the top-level `std::move` looks redundant, maybe remove it?



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:31
+
+using ::clang::ast_matchers::MatchFinder;
+using ::clang::ast_type_traits::ASTNodeKind;

NIT: we could leave out the trailing `::` for `clang` and `llvm`, they are 
well-known and unambiguous namespace names.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:42
+
+static bool isOriginMacroBody(const clang::SourceManager ,
+  clang::SourceLocation Loc) {

NIT: could you add a brief comment on what this function is expected to return?



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:133
+
+namespace clang {
+namespace tooling {

NIT: remove namespaces for consistency with the rest of the code.

(Probably a leftover from the previous version)



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:150
+  llvm::handleErrors(TargetOrErr.takeError(), [](StringError ) {
+return invalidArgumentError("Failure targeting node" +
+Rule.target() + ": " + E.getMessage());

NIT: consider simply propagating the original error up the stack in that case 
to avoid boilerplate.
Although adding the `.target()` information to the error might be useful, so up 
to you.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:157
+  isOriginMacroBody(*Match.SourceManager, Target.getBegin()))
+return Transformation();
+

Why not return an error in case of macros too? Is there any use of the empty 
transformation to the clients? 
Alternatively, we might want to document this behavior (applyRewriteRule can 
return empty transformations) and it's rationale.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:28
+
+namespace clang {
+namespace tooling {

ymandel wrote:
> ilya-biryukov wrote:
> > Other files in the tooling library seem to be adding `using namespace 
> > clang` instead of putting the declaration into a namespace.
> > Could you please change the new code to do the same for consistency?
> > 
> Done.  Agreed about being consistent. FWIW, I can't say I like this style.  
> Perhaps because I'm not used to it, but it feels too implicit.  It forces the 
> reader to figure out where each definition is being associated. Also, I 
> discovered it only works for method definitions. Free functions still need 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Thanks for the detailed review and really helpful comments!




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:38
+/// @{
+using ast_matchers::CXXCtorInitializerMatcher;
+using ast_matchers::DeclarationMatcher;

ilya-biryukov wrote:
> I'm not sure if this is in the LLVM style guide, but we might want to avoid 
> introducing these names into `clang::tooling` namespaces in the headers.
> 
> My fear is that users will rely on those using without knowing that 
> explicitly and won't add corresponding `using` directives or qualifiers to 
> their `.cpp` files, making refactoring and moving the code around harder.
> 
> Could you fully-qualify those names in the header instead? There does not 
> seem to be too many of them.
right. I'd intended to introduce these into the clang tooling namespace -- that 
is, these weren't just convenience aliases for the header file. But, I no 
longer think that's useful in any case, so dropping them is certainly best.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:65
+
+using TextGenerator = std::function(
+const ast_matchers::MatchFinder::MatchResult &)>;

ilya-biryukov wrote:
> Why would a `TextGenerator`  fail?
> I imagine all of the failure cases are programming errors (matchers in the 
> rewrite rule were not aligned with the corresponding text generating 
> function). For those cases, using the `assert` macro seems cleaner.
Sure.  I could go either way. I think some of these cases fall on the border 
between an invariant violation and "invalid argument" or some such.  But, let's 
keep it simpler for now.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:116
+  // never be the empty string.
+  std::string Target = RootId;
+  ast_type_traits::ASTNodeKind TargetKind;

ilya-biryukov wrote:
> NIT: maybe move all inits to the constructor?
> To have all initializers in one place.
nicer, thanks.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:170
+
+  RewriteRuleBuilder(const RewriteRuleBuilder &) = default;
+  RewriteRuleBuilder(RewriteRuleBuilder &&) = default;

ilya-biryukov wrote:
> NIT: maybe remove the `=default` copy and move ctors and assignments?
> They should be generated automatically anyway, right?
Sure. I was going based on google's style recommendations, but personally i 
prefer leaving them implicit.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:28
+
+namespace clang {
+namespace tooling {

ilya-biryukov wrote:
> Other files in the tooling library seem to be adding `using namespace clang` 
> instead of putting the declaration into a namespace.
> Could you please change the new code to do the same for consistency?
> 
Done.  Agreed about being consistent. FWIW, I can't say I like this style.  
Perhaps because I'm not used to it, but it feels too implicit.  It forces the 
reader to figure out where each definition is being associated. Also, I 
discovered it only works for method definitions. Free functions still need to 
be explicitly namespaced.

Any idea what the reason for this style is?



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:30
+namespace tooling {
+namespace {
+using ::clang::ast_matchers::MatchFinder;

ilya-biryukov wrote:
> Why put using directives into an anonymous namespace?
> I have not seen this pattern before, could you point me to explanations on 
> why this is useful?
You gain an extra little bit of robustness against clashing with something 
declared in the enclosing namespace.  But, this is overkill even for me -- I 
generally only do this when I already have an anonymous namespace; there's no 
good reason to create one for this purpose.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:111
+// Requires verifyTarget(node, target_part) == success.
+static CharSourceRange getTarget(const DynTypedNode , ASTNodeKind Kind,
+ NodePart TargetPart, ASTContext ) {

ilya-biryukov wrote:
> NIT: consider merging `verifyTarget` into `getTarget` and making `getTarget` 
> return `Expected`.
> Would allow avoiding to write one of the complicated switches and 
> error-checking arguably looks just as natural in the `getTarget` as it is in 
> `verifyTarget`.
> 
> Also, having the invariants closer to the code using them makes it easier to 
> certify both are correct, e.g. seeing that `NamedDecl.isIdentifier()` was 
> checked before accessing the `NamedDecl.getName()` in the same function is 
> simpler.
Yes, I like it much better this way.  The split wasn't worth it.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:134
+  auto R = CharSourceRange::getTokenRange(TokenLoc, TokenLoc);
+  // Verify that the range covers exactly the 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 192623.
ymandel marked 26 inline comments as done.
ymandel added a comment.

Adjusted test to cover case discussed in comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -0,0 +1,418 @@
+//===- unittest/Tooling/TransformerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ::clang::ast_matchers::anyOf;
+using ::clang::ast_matchers::argumentCountIs;
+using ::clang::ast_matchers::callee;
+using ::clang::ast_matchers::callExpr;
+using ::clang::ast_matchers::cxxMemberCallExpr;
+using ::clang::ast_matchers::cxxMethodDecl;
+using ::clang::ast_matchers::cxxRecordDecl;
+using ::clang::ast_matchers::declRefExpr;
+using ::clang::ast_matchers::expr;
+using ::clang::ast_matchers::functionDecl;
+using ::clang::ast_matchers::hasAnyName;
+using ::clang::ast_matchers::hasArgument;
+using ::clang::ast_matchers::hasDeclaration;
+using ::clang::ast_matchers::hasElse;
+using ::clang::ast_matchers::hasName;
+using ::clang::ast_matchers::hasType;
+using ::clang::ast_matchers::ifStmt;
+using ::clang::ast_matchers::member;
+using ::clang::ast_matchers::memberExpr;
+using ::clang::ast_matchers::namedDecl;
+using ::clang::ast_matchers::on;
+using ::clang::ast_matchers::pointsTo;
+using ::clang::ast_matchers::to;
+using ::clang::ast_matchers::unless;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+string(const char*);
+char* c_str();
+int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+} // namespace
+
+static clang::ast_matchers::internal::Matcher
+isOrPointsTo(const clang::ast_matchers::DeclarationMatcher ) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+static std::string format(llvm::StringRef Code) {
+  const std::vector Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+ADD_FAILURE() << "Could not format code: "
+  << llvm::toString(Formatted.takeError());
+return std::string();
+  }
+  return *Formatted;
+}
+
+void compareSnippets(llvm::StringRef Expected,
+ const llvm::Optional ) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos) {
+Actual.erase(I, HL.size());
+  }
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(llvm::StringRef S) { FileContents[0].second += S; }
+
+  void addFile(llvm::StringRef Filename, llvm::StringRef Content) {
+FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional rewrite(llvm::StringRef Input) {
+std::string Code = ("#include \"header.h\"\n" + Input).str();
+auto Factory = newFrontendActionFactory();
+if (!runToolOnCodeWithArgs(
+Factory->create(), Code, std::vector(), "input.cc",
+"clang-tool", std::make_shared(),
+FileContents)) {
+  return None;
+}
+auto ChangedCodeOrErr =
+applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+if (auto Err = ChangedCodeOrErr.takeError()) {
+  llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
+   << "\n";
+  return None;
+}
+return *ChangedCodeOrErr;
+  }
+
+  clang::ast_matchers::MatchFinder MatchFinder;
+  AtomicChanges Changes;
+
+private:
+  FileContentMappings FileContents = {{"header.h", ""}};
+};
+
+class TransformerTest : public ClangRefactoringTestBase {

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 192619.
ymandel marked 2 inline comments as done.
ymandel added a comment.

Simplified TextGenerator, changed namespace handling, collapsed verifyTarget() 
into getTarget(), and other changes in response to comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -0,0 +1,416 @@
+//===- unittest/Tooling/TransformerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ::clang::ast_matchers::anyOf;
+using ::clang::ast_matchers::argumentCountIs;
+using ::clang::ast_matchers::callee;
+using ::clang::ast_matchers::callExpr;
+using ::clang::ast_matchers::cxxMemberCallExpr;
+using ::clang::ast_matchers::cxxMethodDecl;
+using ::clang::ast_matchers::cxxRecordDecl;
+using ::clang::ast_matchers::declRefExpr;
+using ::clang::ast_matchers::expr;
+using ::clang::ast_matchers::functionDecl;
+using ::clang::ast_matchers::hasAnyName;
+using ::clang::ast_matchers::hasArgument;
+using ::clang::ast_matchers::hasDeclaration;
+using ::clang::ast_matchers::hasElse;
+using ::clang::ast_matchers::hasName;
+using ::clang::ast_matchers::hasType;
+using ::clang::ast_matchers::ifStmt;
+using ::clang::ast_matchers::member;
+using ::clang::ast_matchers::memberExpr;
+using ::clang::ast_matchers::namedDecl;
+using ::clang::ast_matchers::on;
+using ::clang::ast_matchers::pointsTo;
+using ::clang::ast_matchers::to;
+using ::clang::ast_matchers::unless;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+string(const char*);
+char* c_str();
+int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+} // namespace
+
+static clang::ast_matchers::internal::Matcher
+isOrPointsTo(const clang::ast_matchers::DeclarationMatcher ) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+static std::string format(llvm::StringRef Code) {
+  const std::vector Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+ADD_FAILURE() << "Could not format code: "
+  << llvm::toString(Formatted.takeError());
+return std::string();
+  }
+  return *Formatted;
+}
+
+void compareSnippets(llvm::StringRef Expected,
+ const llvm::Optional ) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos) {
+Actual.erase(I, HL.size());
+  }
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(llvm::StringRef S) { FileContents[0].second += S; }
+
+  void addFile(llvm::StringRef Filename, llvm::StringRef Content) {
+FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional rewrite(llvm::StringRef Input) {
+std::string Code = ("#include \"header.h\"\n" + Input).str();
+auto Factory = newFrontendActionFactory();
+if (!runToolOnCodeWithArgs(
+Factory->create(), Code, std::vector(), "input.cc",
+"clang-tool", std::make_shared(),
+FileContents)) {
+  return None;
+}
+auto ChangedCodeOrErr =
+applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+if (auto Err = ChangedCodeOrErr.takeError()) {
+  llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
+   << "\n";
+  return None;
+}
+return *ChangedCodeOrErr;
+  }
+
+  clang::ast_matchers::MatchFinder MatchFinder;
+  AtomicChanges Changes;
+
+private:
+  FileContentMappings FileContents = 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

Mostly nits and small API questions form my side.
This looks very good overall! I'm planning to take a closer look at the 
handling of macros and various AST nodes in more detail this week, but the 
approach looks solid from a higher level.




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:135
+// \endcode
+class RewriteRule {
+public:

ymandel wrote:
> ilya-biryukov wrote:
> > ymandel wrote:
> > > ilya-biryukov wrote:
> > > > ymandel wrote:
> > > > > ilya-biryukov wrote:
> > > > > > Maybe consider separating the fluent API to build the rewrite rule 
> > > > > > from the rewrite rule itself?
> > > > > > 
> > > > > > Not opposed to having the fluent builder API, this does look nice 
> > > > > > and seems to nicely align with the matcher APIs.
> > > > > > However, it is somewhat hard to figure out what can `RewriteRule` 
> > > > > > do **after** it was built when looking at the code right now.
> > > > > > ```
> > > > > > class RewriteRule {
> > > > > > public:
> > > > > >   RewriteRule(DynTypedMatcher, TextGenerator Replacement, 
> > > > > > TextGenerator Explanation);
> > > > > > 
> > > > > >   DynTypedMatcher matcher() const;
> > > > > >   Expected replacement() const;
> > > > > >   Expected explanation() const;
> > > > > > };
> > > > > > 
> > > > > > struct RewriteRuleBuilder { // Having a better name than 'Builder' 
> > > > > > would be nice.
> > > > > >   RewriteRule finish() &&; // produce the final RewriteRule.
> > > > > > 
> > > > > >   template 
> > > > > >   RewriteRuleBuilder (const TypedNodeId ,
> > > > > >   NodePart Part = NodePart::Node) &;
> > > > > >   RewriteRuleBuilder (TextGenerator Replacement) &;
> > > > > >   RewriteRuleBuilder (TextGenerator Explanation) &;
> > > > > > private:
> > > > > >   RewriteRule RuleInProgress;
> > > > > > };
> > > > > > RewriteRuleBuilder makeRewriteRule();
> > > > > > ```
> > > > > I see your point, but do you think it might be enough to improve the 
> > > > > comments on the class? My concern with a builder is the mental burden 
> > > > > on the user of another concept (the builder) and the need for an 
> > > > > extra `.build()` everywhere. To a lesser extent, I also don't love 
> > > > > the cost of an extra copy, although I doubt it matters and I suppose 
> > > > > we could support moves in the build method.
> > > > The issues with the builder interface is that it requires lots of 
> > > > boilerplate, which is hard to throw away when reading the code of the 
> > > > class. I agree that having a separate builder class is also annoying 
> > > > (more concepts, etc).
> > > > 
> > > > Keeping them separate would be my personal preference, but if you'd 
> > > > prefer to keep it in the same class than maybe move the non-builder 
> > > > pieces to the top of the class and separate the builder methods with a 
> > > > comment. 
> > > > WDYT? 
> > > > 
> > > > > To a lesser extent, I also don't love the cost of an extra copy, 
> > > > > although I doubt it matters and I suppose we could support moves in 
> > > > > the build method.
> > > > I believe we can be as efficient (up to an extra move) with builders as 
> > > > without them. If most usages are of the form `RewriteRule R = 
> > > > rewriteRule(...).change(...).replaceWith(...).because(...);`
> > > > Then we could make all builder functions return r-value reference to a 
> > > > builder and have an implicit conversion operator that would consume the 
> > > > builder, producing the final `RewriteRule`:
> > > > ```
> > > > class RewriteRuleBuilder {
> > > >   operator RewriteRule () &&;
> > > >   /// ...
> > > > };
> > > > RewriteRuleBuilder rewriteRule();
> > > > 
> > > > void addRule(RewriteRule R);
> > > > void clientCode() {
> > > >   
> > > > addRule(rewriteRule().change(Matcher).replaceWith("foo").because("bar"));
> > > > }
> > > > ```
> > > I didn't realize that implicit conversion functions are allowed. With 
> > > that, I'm fine w/ splitting.   Thanks!
> > Have you uploaded the new version? I don't seem to see the split.
> I have now.  FWIW, I've left both ref overloads, but what do you think of 
> dropping the lvalue-ref overloads and only supporting rvalue refs?  Users can 
> still pretty easily use an lvalue, just by inserting a trivial std::move() 
> around the lvalue.  
Yeah, LG, having only a single overload means less boilerplate and the 
fluent-builder APIs are mostly used exclusively as r-values anyway.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:38
+/// @{
+using ast_matchers::CXXCtorInitializerMatcher;
+using ast_matchers::DeclarationMatcher;

I'm not sure if this is in the LLVM style guide, but we might want to avoid 
introducing these names into `clang::tooling` namespaces in the headers.

My fear is that users will rely on those using without knowing that explicitly 
and won't 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked an inline comment as done.
ymandel added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+  /// relevant name, not including qualifiers.
+  Name,
+};

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > Same here, what happens to the template arguments and multi-token names, 
> > > e.g.
> > > `operator +` or `foo`?
> > Good point. This seems difficult to get right, since NamedDecl does not 
> > carry sufficient loc data.  However, I've updated the code to explicitly 
> > fail in such cases, so at least we won't have bad rewrites.
> > 
> > BTW, any idea whether constructor initializers can ever be multi token?
> > BTW, any idea whether constructor initializers can ever be multi token?
> 
> Two cases come to mind:
> 1. arbitrary names when initializing base classes,  something like 
> `::ns::X(10)`
> 2. template packs with ellipsis (although ellipsis shouldn't be normally part 
> that we replace, I guess): `Base(10)...`
> 
> Full example:
> ```
> namespace ns {
>   struct X {
> X(int);
>   };
> }
> 
> 
> template 
> struct Y : ns::X, Bases... {
>   Y() : ns::X(10), Bases(10)... {
>   }
> };
> 
> struct Z {
>   Z(int);
> };
> struct W {
>   W(int);
> };
> 
> Y y;
> ```
Turns out the code was already filtering these cases. I added an addition 
constrain of I->isWritten() for initializers. So, only explicit initialization 
of fields is allowed, and therefore I'd venture guaranteed to be a single 
token. I noticed that Kythe seems to make the same assumption.  

That said, I could change to code to specify the range as a char-range of 
`getMemberLoc(), getLParenLoc()` if we can't rely on that (single-token) 
guarantee.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 192267.
ymandel marked 5 inline comments as done.
ymandel added a comment.

- Remove lvalue-ref overloads in builder
- add StringRef overloads for TextGenerator-taking methods
- constrain Name targeting for ctor initializers to explicitly written 
initializers.
- add multi-token member test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -0,0 +1,416 @@
+//===- unittest/Tooling/TransformerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ::clang::ast_matchers::anyOf;
+using ::clang::ast_matchers::argumentCountIs;
+using ::clang::ast_matchers::callee;
+using ::clang::ast_matchers::callExpr;
+using ::clang::ast_matchers::cxxMemberCallExpr;
+using ::clang::ast_matchers::cxxMethodDecl;
+using ::clang::ast_matchers::cxxRecordDecl;
+using ::clang::ast_matchers::declRefExpr;
+using ::clang::ast_matchers::expr;
+using ::clang::ast_matchers::functionDecl;
+using ::clang::ast_matchers::hasAnyName;
+using ::clang::ast_matchers::hasArgument;
+using ::clang::ast_matchers::hasDeclaration;
+using ::clang::ast_matchers::hasElse;
+using ::clang::ast_matchers::hasName;
+using ::clang::ast_matchers::hasType;
+using ::clang::ast_matchers::ifStmt;
+using ::clang::ast_matchers::member;
+using ::clang::ast_matchers::memberExpr;
+using ::clang::ast_matchers::namedDecl;
+using ::clang::ast_matchers::on;
+using ::clang::ast_matchers::pointsTo;
+using ::clang::ast_matchers::to;
+using ::clang::ast_matchers::unless;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+string(const char*);
+char* c_str();
+int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+} // namespace
+
+static clang::ast_matchers::internal::Matcher
+isOrPointsTo(const DeclarationMatcher ) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+static std::string format(llvm::StringRef Code) {
+  const std::vector Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+ADD_FAILURE() << "Could not format code: "
+  << llvm::toString(Formatted.takeError());
+return std::string();
+  }
+  return *Formatted;
+}
+
+void compareSnippets(llvm::StringRef Expected,
+ const llvm::Optional ) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos) {
+Actual.erase(I, HL.size());
+  }
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(llvm::StringRef S) { FileContents[0].second += S; }
+
+  void addFile(llvm::StringRef Filename, llvm::StringRef Content) {
+FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional rewrite(llvm::StringRef Input) {
+std::string Code = ("#include \"header.h\"\n" + Input).str();
+auto Factory = newFrontendActionFactory();
+if (!runToolOnCodeWithArgs(
+Factory->create(), Code, std::vector(), "input.cc",
+"clang-tool", std::make_shared(),
+FileContents)) {
+  return None;
+}
+auto ChangedCodeOrErr =
+applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+if (auto Err = ChangedCodeOrErr.takeError()) {
+  llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
+   << "\n";
+  return None;
+}
+return *ChangedCodeOrErr;
+  }
+
+  clang::ast_matchers::MatchFinder MatchFinder;
+  AtomicChanges Changes;

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+  /// relevant name, not including qualifiers.
+  Name,
+};

ymandel wrote:
> ilya-biryukov wrote:
> > Same here, what happens to the template arguments and multi-token names, 
> > e.g.
> > `operator +` or `foo`?
> Good point. This seems difficult to get right, since NamedDecl does not carry 
> sufficient loc data.  However, I've updated the code to explicitly fail in 
> such cases, so at least we won't have bad rewrites.
> 
> BTW, any idea whether constructor initializers can ever be multi token?
> BTW, any idea whether constructor initializers can ever be multi token?

Two cases come to mind:
1. arbitrary names when initializing base classes,  something like 
`::ns::X(10)`
2. template packs with ellipsis (although ellipsis shouldn't be normally part 
that we replace, I guess): `Base(10)...`

Full example:
```
namespace ns {
  struct X {
X(int);
  };
}


template 
struct Y : ns::X, Bases... {
  Y() : ns::X(10), Bases(10)... {
  }
};

struct Z {
  Z(int);
};
struct W {
  W(int);
};

Y y;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 10 inline comments as done.
ymandel added a comment.

Addressed the most major comments. Still working on some smaller ones. PTAL.  
Thanks!




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:54
+/// boolean expression language for constructing filters.
+class MatchFilter {
+public:

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > Intuitively, it feels that any filtering should be possible at the level 
> > > of the AST matchers. Is that not the case?
> > > Could you provide some motivating examples where AST matchers cannot be 
> > > used to nail down the matching nodes and we need `MatchFilter`? 
> > > 
> > > Please note I have limited experience with AST matchers, so there might 
> > > be some obvious things that I'm missing.
> > Good point. The examples I have would actually be perfectly suited to a 
> > matcher.  That said, there is not matcher support for a simple predicate of 
> > this form, along the lines of gtest's `Truly(predicate)`. I'll remove this 
> > and separately try to add something like `Truly` to the matchers.
> Makes sense! Maybe put a `FIXME` here to let the readers know this is moving 
> to the ast matchers?
I've removed the where clause entirely. I'll separately add the matcher support 
(I've figured out how to do it within the existing framework).



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:85
+  Node,
+  /// Given a \c MemberExpr, selects the member's token.
+  Member,

ilya-biryukov wrote:
> What happens if member is composed of multiple tokens? E.g.
> ```
> foo.bar::baz; // member tokens are ['bar', '::', 'baz']
> foo.template baz; // member tokens are ['template', 'baz', '<', 'int', 
> '>']
> foo.operator +; // member tokens are ['operator', '+']
> ```
> 
> I might be misinterpreting the meaning of "member" token.
Good catch! I've updated to handle these correctly (I believe). Added some 
tests, plan to add some more.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+  /// relevant name, not including qualifiers.
+  Name,
+};

ilya-biryukov wrote:
> Same here, what happens to the template arguments and multi-token names, e.g.
> `operator +` or `foo`?
Good point. This seems difficult to get right, since NamedDecl does not carry 
sufficient loc data.  However, I've updated the code to explicitly fail in such 
cases, so at least we won't have bad rewrites.

BTW, any idea whether constructor initializers can ever be multi token?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:135
+// \endcode
+class RewriteRule {
+public:

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > ymandel wrote:
> > > > ilya-biryukov wrote:
> > > > > Maybe consider separating the fluent API to build the rewrite rule 
> > > > > from the rewrite rule itself?
> > > > > 
> > > > > Not opposed to having the fluent builder API, this does look nice and 
> > > > > seems to nicely align with the matcher APIs.
> > > > > However, it is somewhat hard to figure out what can `RewriteRule` do 
> > > > > **after** it was built when looking at the code right now.
> > > > > ```
> > > > > class RewriteRule {
> > > > > public:
> > > > >   RewriteRule(DynTypedMatcher, TextGenerator Replacement, 
> > > > > TextGenerator Explanation);
> > > > > 
> > > > >   DynTypedMatcher matcher() const;
> > > > >   Expected replacement() const;
> > > > >   Expected explanation() const;
> > > > > };
> > > > > 
> > > > > struct RewriteRuleBuilder { // Having a better name than 'Builder' 
> > > > > would be nice.
> > > > >   RewriteRule finish() &&; // produce the final RewriteRule.
> > > > > 
> > > > >   template 
> > > > >   RewriteRuleBuilder (const TypedNodeId ,
> > > > >   NodePart Part = NodePart::Node) &;
> > > > >   RewriteRuleBuilder (TextGenerator Replacement) &;
> > > > >   RewriteRuleBuilder (TextGenerator Explanation) &;
> > > > > private:
> > > > >   RewriteRule RuleInProgress;
> > > > > };
> > > > > RewriteRuleBuilder makeRewriteRule();
> > > > > ```
> > > > I see your point, but do you think it might be enough to improve the 
> > > > comments on the class? My concern with a builder is the mental burden 
> > > > on the user of another concept (the builder) and the need for an extra 
> > > > `.build()` everywhere. To a lesser extent, I also don't love the cost 
> > > > of an extra copy, although I doubt it matters and I suppose we could 
> > > > support moves in the build method.
> > > The issues with the builder interface is that it requires lots of 
> > > boilerplate, which is hard to throw away when reading the code of the 
> > > class. I agree that having a separate builder class is also annoying 
> > > (more concepts, etc).
> > > 
> > > Keeping them separate would be my personal preference, but if you'd 
> > > prefer to keep it in the same 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 192172.
ymandel marked an inline comment as done.
ymandel added a comment.

Split RewriteRule into two classes, remove support for where clause, support 
multi-token targets, and add corresponding tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -0,0 +1,447 @@
+//===- unittest/Tooling/TransformerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ::clang::ast_matchers::anyOf;
+using ::clang::ast_matchers::argumentCountIs;
+using ::clang::ast_matchers::callee;
+using ::clang::ast_matchers::callExpr;
+using ::clang::ast_matchers::cxxMemberCallExpr;
+using ::clang::ast_matchers::cxxMethodDecl;
+using ::clang::ast_matchers::cxxRecordDecl;
+using ::clang::ast_matchers::declRefExpr;
+using ::clang::ast_matchers::expr;
+using ::clang::ast_matchers::functionDecl;
+using ::clang::ast_matchers::hasAnyName;
+using ::clang::ast_matchers::hasArgument;
+using ::clang::ast_matchers::hasDeclaration;
+using ::clang::ast_matchers::hasElse;
+using ::clang::ast_matchers::hasName;
+using ::clang::ast_matchers::hasType;
+using ::clang::ast_matchers::ifStmt;
+using ::clang::ast_matchers::member;
+using ::clang::ast_matchers::memberExpr;
+using ::clang::ast_matchers::namedDecl;
+using ::clang::ast_matchers::on;
+using ::clang::ast_matchers::pointsTo;
+using ::clang::ast_matchers::to;
+using ::clang::ast_matchers::unless;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+string(const char*);
+char* c_str();
+int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+} // namespace
+
+static clang::ast_matchers::internal::Matcher
+isOrPointsTo(const DeclarationMatcher ) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+static std::string format(llvm::StringRef Code) {
+  const std::vector Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+ADD_FAILURE() << "Could not format code: "
+  << llvm::toString(Formatted.takeError());
+return std::string();
+  }
+  return *Formatted;
+}
+
+void compareSnippets(llvm::StringRef Expected,
+ const llvm::Optional ) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos) {
+Actual.erase(I, HL.size());
+  }
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(llvm::StringRef S) { FileContents[0].second += S; }
+
+  void addFile(llvm::StringRef Filename, llvm::StringRef Content) {
+FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional rewrite(llvm::StringRef Input) {
+std::string Code = ("#include \"header.h\"\n" + Input).str();
+auto Factory = newFrontendActionFactory();
+if (!runToolOnCodeWithArgs(
+Factory->create(), Code, std::vector(), "input.cc",
+"clang-tool", std::make_shared(),
+FileContents)) {
+  return None;
+}
+auto ChangedCodeOrErr =
+applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+if (auto Err = ChangedCodeOrErr.takeError()) {
+  llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
+   << "\n";
+  return None;
+}
+return *ChangedCodeOrErr;
+  }
+
+  clang::ast_matchers::MatchFinder MatchFinder;
+  AtomicChanges Changes;
+
+private:
+  FileContentMappings FileContents = {{"header.h", ""}};
+};
+
+class 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D59376#1439928 , @ymandel wrote:

> Working on the new version now. Will note with "PTAL" once that's ready.


Got you! Waiting for the new revision.

> Sorry that wasn't clear in earlier responses.

Or maybe it's me misreading earlier responses. Happy to review the new changes 
as soon as they land.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked an inline comment as done.
ymandel added a comment.

Working on the new version now. Will note with "PTAL" once that's ready. Sorry 
that wasn't clear in earlier responses.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/unittests/Tooling/TransformerTest.cpp:157
+  .as()
+  .replaceWith(text("REPLACED"))
+  .because(text("Use size() method directly on string."));

NIT: maybe consider adding overloads for these function that allow to pass char 
literals there? This is probably a common case, so it'd be nice to write this 
as:
```
replaceWith("REPLACED").because("Use size() method directly on string.");
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I've just realized the review is probably missing the latest revision.
@ymandel, could you upload the new version?




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:54
+/// boolean expression language for constructing filters.
+class MatchFilter {
+public:

ymandel wrote:
> ilya-biryukov wrote:
> > Intuitively, it feels that any filtering should be possible at the level of 
> > the AST matchers. Is that not the case?
> > Could you provide some motivating examples where AST matchers cannot be 
> > used to nail down the matching nodes and we need `MatchFilter`? 
> > 
> > Please note I have limited experience with AST matchers, so there might be 
> > some obvious things that I'm missing.
> Good point. The examples I have would actually be perfectly suited to a 
> matcher.  That said, there is not matcher support for a simple predicate of 
> this form, along the lines of gtest's `Truly(predicate)`. I'll remove this 
> and separately try to add something like `Truly` to the matchers.
Makes sense! Maybe put a `FIXME` here to let the readers know this is moving to 
the ast matchers?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:57
+  using Predicate =
+  std::function;
+

Note: I'm probably not seeing the latest version, so this comment might be 
outdated. Feel free to ignore if you've already moved this to the matchers.

BTW, I was wondering whether `MatchFilter` carries its weight? What are the 
pros over: 
```
using MatchFilter = std::function;
```

The only thing thats come to mind is that default-constructed function cannot 
be called, which is less useful than returning true in a default-constructed.
We could handle this in the builder once, the rewrite rule can assert the 
predicate is always set. 

Do you have other reasons to have it in mind that I'm missing?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:85
+  Node,
+  /// Given a \c MemberExpr, selects the member's token.
+  Member,

What happens if member is composed of multiple tokens? E.g.
```
foo.bar::baz; // member tokens are ['bar', '::', 'baz']
foo.template baz; // member tokens are ['template', 'baz', '<', 'int', '>']
foo.operator +; // member tokens are ['operator', '+']
```

I might be misinterpreting the meaning of "member" token.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+  /// relevant name, not including qualifiers.
+  Name,
+};

Same here, what happens to the template arguments and multi-token names, e.g.
`operator +` or `foo`?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:135
+// \endcode
+class RewriteRule {
+public:

ymandel wrote:
> ilya-biryukov wrote:
> > ymandel wrote:
> > > ilya-biryukov wrote:
> > > > Maybe consider separating the fluent API to build the rewrite rule from 
> > > > the rewrite rule itself?
> > > > 
> > > > Not opposed to having the fluent builder API, this does look nice and 
> > > > seems to nicely align with the matcher APIs.
> > > > However, it is somewhat hard to figure out what can `RewriteRule` do 
> > > > **after** it was built when looking at the code right now.
> > > > ```
> > > > class RewriteRule {
> > > > public:
> > > >   RewriteRule(DynTypedMatcher, TextGenerator Replacement, TextGenerator 
> > > > Explanation);
> > > > 
> > > >   DynTypedMatcher matcher() const;
> > > >   Expected replacement() const;
> > > >   Expected explanation() const;
> > > > };
> > > > 
> > > > struct RewriteRuleBuilder { // Having a better name than 'Builder' 
> > > > would be nice.
> > > >   RewriteRule finish() &&; // produce the final RewriteRule.
> > > > 
> > > >   template 
> > > >   RewriteRuleBuilder (const TypedNodeId ,
> > > >   NodePart Part = NodePart::Node) &;
> > > >   RewriteRuleBuilder (TextGenerator Replacement) &;
> > > >   RewriteRuleBuilder (TextGenerator Explanation) &;
> > > > private:
> > > >   RewriteRule RuleInProgress;
> > > > };
> > > > RewriteRuleBuilder makeRewriteRule();
> > > > ```
> > > I see your point, but do you think it might be enough to improve the 
> > > comments on the class? My concern with a builder is the mental burden on 
> > > the user of another concept (the builder) and the need for an extra 
> > > `.build()` everywhere. To a lesser extent, I also don't love the cost of 
> > > an extra copy, although I doubt it matters and I suppose we could support 
> > > moves in the build method.
> > The issues with the builder interface is that it requires lots of 
> > boilerplate, which is hard to throw away when reading the code of the 
> > class. I agree that having a separate builder class is also annoying (more 
> > concepts, etc).
> > 
> > Keeping them separate would be my personal preference, but if you'd prefer 
> > to keep it in the same class than maybe 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 4 inline comments as done.
ymandel added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:135
+// \endcode
+class RewriteRule {
+public:

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > Maybe consider separating the fluent API to build the rewrite rule from 
> > > the rewrite rule itself?
> > > 
> > > Not opposed to having the fluent builder API, this does look nice and 
> > > seems to nicely align with the matcher APIs.
> > > However, it is somewhat hard to figure out what can `RewriteRule` do 
> > > **after** it was built when looking at the code right now.
> > > ```
> > > class RewriteRule {
> > > public:
> > >   RewriteRule(DynTypedMatcher, TextGenerator Replacement, TextGenerator 
> > > Explanation);
> > > 
> > >   DynTypedMatcher matcher() const;
> > >   Expected replacement() const;
> > >   Expected explanation() const;
> > > };
> > > 
> > > struct RewriteRuleBuilder { // Having a better name than 'Builder' would 
> > > be nice.
> > >   RewriteRule finish() &&; // produce the final RewriteRule.
> > > 
> > >   template 
> > >   RewriteRuleBuilder (const TypedNodeId ,
> > >   NodePart Part = NodePart::Node) &;
> > >   RewriteRuleBuilder (TextGenerator Replacement) &;
> > >   RewriteRuleBuilder (TextGenerator Explanation) &;
> > > private:
> > >   RewriteRule RuleInProgress;
> > > };
> > > RewriteRuleBuilder makeRewriteRule();
> > > ```
> > I see your point, but do you think it might be enough to improve the 
> > comments on the class? My concern with a builder is the mental burden on 
> > the user of another concept (the builder) and the need for an extra 
> > `.build()` everywhere. To a lesser extent, I also don't love the cost of an 
> > extra copy, although I doubt it matters and I suppose we could support 
> > moves in the build method.
> The issues with the builder interface is that it requires lots of 
> boilerplate, which is hard to throw away when reading the code of the class. 
> I agree that having a separate builder class is also annoying (more concepts, 
> etc).
> 
> Keeping them separate would be my personal preference, but if you'd prefer to 
> keep it in the same class than maybe move the non-builder pieces to the top 
> of the class and separate the builder methods with a comment. 
> WDYT? 
> 
> > To a lesser extent, I also don't love the cost of an extra copy, although I 
> > doubt it matters and I suppose we could support moves in the build method.
> I believe we can be as efficient (up to an extra move) with builders as 
> without them. If most usages are of the form `RewriteRule R = 
> rewriteRule(...).change(...).replaceWith(...).because(...);`
> Then we could make all builder functions return r-value reference to a 
> builder and have an implicit conversion operator that would consume the 
> builder, producing the final `RewriteRule`:
> ```
> class RewriteRuleBuilder {
>   operator RewriteRule () &&;
>   /// ...
> };
> RewriteRuleBuilder rewriteRule();
> 
> void addRule(RewriteRule R);
> void clientCode() {
>   addRule(rewriteRule().change(Matcher).replaceWith("foo").because("bar"));
> }
> ```
I didn't realize that implicit conversion functions are allowed. With that, I'm 
fine w/ splitting.   Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:135
+// \endcode
+class RewriteRule {
+public:

ymandel wrote:
> ilya-biryukov wrote:
> > Maybe consider separating the fluent API to build the rewrite rule from the 
> > rewrite rule itself?
> > 
> > Not opposed to having the fluent builder API, this does look nice and seems 
> > to nicely align with the matcher APIs.
> > However, it is somewhat hard to figure out what can `RewriteRule` do 
> > **after** it was built when looking at the code right now.
> > ```
> > class RewriteRule {
> > public:
> >   RewriteRule(DynTypedMatcher, TextGenerator Replacement, TextGenerator 
> > Explanation);
> > 
> >   DynTypedMatcher matcher() const;
> >   Expected replacement() const;
> >   Expected explanation() const;
> > };
> > 
> > struct RewriteRuleBuilder { // Having a better name than 'Builder' would be 
> > nice.
> >   RewriteRule finish() &&; // produce the final RewriteRule.
> > 
> >   template 
> >   RewriteRuleBuilder (const TypedNodeId ,
> >   NodePart Part = NodePart::Node) &;
> >   RewriteRuleBuilder (TextGenerator Replacement) &;
> >   RewriteRuleBuilder (TextGenerator Explanation) &;
> > private:
> >   RewriteRule RuleInProgress;
> > };
> > RewriteRuleBuilder makeRewriteRule();
> > ```
> I see your point, but do you think it might be enough to improve the comments 
> on the class? My concern with a builder is the mental burden on the user of 
> another concept (the builder) and the need for an extra `.build()` 
> everywhere. To a lesser extent, I also don't love the cost of an extra copy, 
> although I doubt it matters and I suppose we could support moves in the build 
> method.
The issues with the builder interface is that it requires lots of boilerplate, 
which is hard to throw away when reading the code of the class. I agree that 
having a separate builder class is also annoying (more concepts, etc).

Keeping them separate would be my personal preference, but if you'd prefer to 
keep it in the same class than maybe move the non-builder pieces to the top of 
the class and separate the builder methods with a comment. 
WDYT? 

> To a lesser extent, I also don't love the cost of an extra copy, although I 
> doubt it matters and I suppose we could support moves in the build method.
I believe we can be as efficient (up to an extra move) with builders as without 
them. If most usages are of the form `RewriteRule R = 
rewriteRule(...).change(...).replaceWith(...).because(...);`
Then we could make all builder functions return r-value reference to a builder 
and have an implicit conversion operator that would consume the builder, 
producing the final `RewriteRule`:
```
class RewriteRuleBuilder {
  operator RewriteRule () &&;
  /// ...
};
RewriteRuleBuilder rewriteRule();

void addRule(RewriteRule R);
void clientCode() {
  addRule(rewriteRule().change(Matcher).replaceWith("foo").because("bar"));
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 4 inline comments as done.
ymandel added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:54
+/// boolean expression language for constructing filters.
+class MatchFilter {
+public:

ilya-biryukov wrote:
> Intuitively, it feels that any filtering should be possible at the level of 
> the AST matchers. Is that not the case?
> Could you provide some motivating examples where AST matchers cannot be used 
> to nail down the matching nodes and we need `MatchFilter`? 
> 
> Please note I have limited experience with AST matchers, so there might be 
> some obvious things that I'm missing.
Good point. The examples I have would actually be perfectly suited to a 
matcher.  That said, there is not matcher support for a simple predicate of 
this form, along the lines of gtest's `Truly(predicate)`. I'll remove this and 
separately try to add something like `Truly` to the matchers.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:135
+// \endcode
+class RewriteRule {
+public:

ilya-biryukov wrote:
> Maybe consider separating the fluent API to build the rewrite rule from the 
> rewrite rule itself?
> 
> Not opposed to having the fluent builder API, this does look nice and seems 
> to nicely align with the matcher APIs.
> However, it is somewhat hard to figure out what can `RewriteRule` do 
> **after** it was built when looking at the code right now.
> ```
> class RewriteRule {
> public:
>   RewriteRule(DynTypedMatcher, TextGenerator Replacement, TextGenerator 
> Explanation);
> 
>   DynTypedMatcher matcher() const;
>   Expected replacement() const;
>   Expected explanation() const;
> };
> 
> struct RewriteRuleBuilder { // Having a better name than 'Builder' would be 
> nice.
>   RewriteRule finish() &&; // produce the final RewriteRule.
> 
>   template 
>   RewriteRuleBuilder (const TypedNodeId ,
>   NodePart Part = NodePart::Node) &;
>   RewriteRuleBuilder (TextGenerator Replacement) &;
>   RewriteRuleBuilder (TextGenerator Explanation) &;
> private:
>   RewriteRule RuleInProgress;
> };
> RewriteRuleBuilder makeRewriteRule();
> ```
I see your point, but do you think it might be enough to improve the comments 
on the class? My concern with a builder is the mental burden on the user of 
another concept (the builder) and the need for an extra `.build()` everywhere. 
To a lesser extent, I also don't love the cost of an extra copy, although I 
doubt it matters and I suppose we could support moves in the build method.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Looks very polished, thanks!
Will have to sink the change in a bit, will come back with more comments on 
Monday.
In the meantime, a few initial comments and suggestions.




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:54
+/// boolean expression language for constructing filters.
+class MatchFilter {
+public:

Intuitively, it feels that any filtering should be possible at the level of the 
AST matchers. Is that not the case?
Could you provide some motivating examples where AST matchers cannot be used to 
nail down the matching nodes and we need `MatchFilter`? 

Please note I have limited experience with AST matchers, so there might be some 
obvious things that I'm missing.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:135
+// \endcode
+class RewriteRule {
+public:

Maybe consider separating the fluent API to build the rewrite rule from the 
rewrite rule itself?

Not opposed to having the fluent builder API, this does look nice and seems to 
nicely align with the matcher APIs.
However, it is somewhat hard to figure out what can `RewriteRule` do **after** 
it was built when looking at the code right now.
```
class RewriteRule {
public:
  RewriteRule(DynTypedMatcher, TextGenerator Replacement, TextGenerator 
Explanation);

  DynTypedMatcher matcher() const;
  Expected replacement() const;
  Expected explanation() const;
};

struct RewriteRuleBuilder { // Having a better name than 'Builder' would be 
nice.
  RewriteRule finish() &&; // produce the final RewriteRule.

  template 
  RewriteRuleBuilder (const TypedNodeId ,
  NodePart Part = NodePart::Node) &;
  RewriteRuleBuilder (TextGenerator Replacement) &;
  RewriteRuleBuilder (TextGenerator Explanation) &;
private:
  RewriteRule RuleInProgress;
};
RewriteRuleBuilder makeRewriteRule();
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59376/new/

https://reviews.llvm.org/D59376



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, jdoerfert, jfb, mgorny.
Herald added a project: clang.
ymandel added a parent revision: D59329: [LibTooling] Add NodeId, a strong type 
for AST-matcher node identifiers..

Adds a basic version of Transformer, a library supporting the concise 
specification of clang-based source-to-source transformations.  A full 
discussion of the end goal can be found on the cfe-dev list with subject "[RFC] 
Easier source-to-source transformations with clang tooling".


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59376

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -0,0 +1,428 @@
+//===- unittest/Tooling/TransformerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ::clang::ast_matchers::anyOf;
+using ::clang::ast_matchers::argumentCountIs;
+using ::clang::ast_matchers::callee;
+using ::clang::ast_matchers::callExpr;
+using ::clang::ast_matchers::cxxMemberCallExpr;
+using ::clang::ast_matchers::cxxMethodDecl;
+using ::clang::ast_matchers::cxxRecordDecl;
+using ::clang::ast_matchers::declRefExpr;
+using ::clang::ast_matchers::expr;
+using ::clang::ast_matchers::functionDecl;
+using ::clang::ast_matchers::hasAnyName;
+using ::clang::ast_matchers::hasArgument;
+using ::clang::ast_matchers::hasDeclaration;
+using ::clang::ast_matchers::hasElse;
+using ::clang::ast_matchers::hasName;
+using ::clang::ast_matchers::hasType;
+using ::clang::ast_matchers::ifStmt;
+using ::clang::ast_matchers::member;
+using ::clang::ast_matchers::memberExpr;
+using ::clang::ast_matchers::namedDecl;
+using ::clang::ast_matchers::on;
+using ::clang::ast_matchers::pointsTo;
+using ::clang::ast_matchers::to;
+using ::clang::ast_matchers::unless;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+string(const char*);
+char* c_str();
+int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+} // namespace
+
+static clang::ast_matchers::internal::Matcher
+isOrPointsTo(const DeclarationMatcher ) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+static std::string format(llvm::StringRef Code) {
+  const std::vector Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+ADD_FAILURE() << "Could not format code: "
+  << llvm::toString(Formatted.takeError());
+return std::string();
+  }
+  return *Formatted;
+}
+
+void compareSnippets(llvm::StringRef Expected,
+ const llvm::Optional ) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos) {
+Actual.erase(I, HL.size());
+  }
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(llvm::StringRef S) { FileContents[0].second += S; }
+
+  void addFile(llvm::StringRef Filename, llvm::StringRef Content) {
+FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional rewrite(llvm::StringRef Input) {
+std::string Code = ("#include \"header.h\"\n" + Input).str();
+auto Factory = newFrontendActionFactory();
+if (!runToolOnCodeWithArgs(
+Factory->create(), Code, std::vector(), "input.cc",
+"clang-tool", std::make_shared(),
+FileContents)) {
+  return None;
+}
+auto ChangedCodeOrErr =
+applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+if (auto Err = ChangedCodeOrErr.takeError()) {
+  llvm::errs() << "Change failed: " <<