[PATCH] D37681: [refactor] Simplify the interface and remove some template magic

[Alex Lorenz via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL314704: [refactor] Simplify the refactoring interface 
(authored by arphaman).

Changed prior to commit:
  https://reviews.llvm.org/D37681?vs=115211=117403#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D37681

Files:
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h
  
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirementsInternal.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRules.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringResultConsumer.h
  cfe/trunk/include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
  cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
  cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  cfe/trunk/lib/Tooling/Refactoring/SourceSelectionConstraints.cpp
  cfe/trunk/unittests/Tooling/RefactoringActionRulesTest.cpp

Index: cfe/trunk/unittests/Tooling/RefactoringActionRulesTest.cpp
===
--- cfe/trunk/unittests/Tooling/RefactoringActionRulesTest.cpp
+++ cfe/trunk/unittests/Tooling/RefactoringActionRulesTest.cpp
@@ -18,7 +18,6 @@
 
 using namespace clang;
 using namespace tooling;
-using namespace refactoring_action_rules;
 
 namespace {
 
@@ -56,29 +55,39 @@
 }
 
 TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
-  auto ReplaceAWithB =
-  [](const RefactoringRuleContext &,
- std::pair Selection)
-  -> Expected {
-const SourceManager  = Selection.first.getSources();
-SourceLocation Loc = Selection.first.getRange().getBegin().getLocWithOffset(
-Selection.second);
-AtomicChange Change(SM, Loc);
-llvm::Error E = Change.replace(SM, Loc, 1, "b");
-if (E)
-  return std::move(E);
-return AtomicChanges{Change};
-  };
-  class SelectionRequirement : public selection::Requirement {
-  public:
-std::pair
-evaluateSelection(const RefactoringRuleContext &,
-  selection::SourceSelectionRange Selection) const {
-  return std::make_pair(Selection, 20);
+  class ReplaceAWithB : public SourceChangeRefactoringRule {
+std::pair Selection;
+
+  public:
+ReplaceAWithB(std::pair Selection)
+: Selection(Selection) {}
+
+Expected
+createSourceReplacements(RefactoringRuleContext ) {
+  const SourceManager  = Context.getSources();
+  SourceLocation Loc =
+  Selection.first.getBegin().getLocWithOffset(Selection.second);
+  AtomicChange Change(SM, Loc);
+  llvm::Error E = Change.replace(SM, Loc, 1, "b");
+  if (E)
+return std::move(E);
+  return AtomicChanges{Change};
+}
+  };
+
+  class SelectionRequirement : public SourceRangeSelectionRequirement {
+  public:
+Expected>
+evaluate(RefactoringRuleContext ) const {
+  Expected R =
+  SourceRangeSelectionRequirement::evaluate(Context);
+  if (!R)
+return R.takeError();
+  return std::make_pair(*R, 20);
 }
   };
-  auto Rule = createRefactoringRule(ReplaceAWithB,
-requiredSelection(SelectionRequirement()));
+  auto Rule =
+  createRefactoringActionRule(SelectionRequirement());
 
   // When the requirements are satisifed, the rule's function must be invoked.
   {
@@ -123,54 +132,24 @@
 llvm::handleAllErrors(
 ErrorOrResult.takeError(),
 [&](llvm::StringError ) { Message = Error.getMessage(); });
-EXPECT_EQ(Message, "refactoring action can't be initiated with the "
-   "specified selection range");
+EXPECT_EQ(Message,
+  "refactoring action can't be initiated without a selection");
   }
 }
 
 TEST_F(RefactoringActionRulesTest, ReturnError) {
-  Expected (*Func)(const RefactoringRuleContext &,
-  selection::SourceSelectionRange) =
-  [](const RefactoringRuleContext &,
- selection::SourceSelectionRange) -> Expected {
-return llvm::make_error(
-"Error", llvm::make_error_code(llvm::errc::invalid_argument));
-  };
-  auto Rule = createRefactoringRule(
-  Func, requiredSelection(
-selection::identity()));
-
-  RefactoringRuleContext RefContext(Context.Sources);
-  SourceLocation Cursor =
-  Context.Sources.getLocForStartOfFile(Context.Sources.getMainFileID());
-  RefContext.setSelectionRange({Cursor, Cursor});
-  Expected Result = createReplacements(Rule, RefContext);
-
-  ASSERT_TRUE(!Result);
-  std::string Message;
-  llvm::handleAllErrors(Result.takeError(), [&](llvm::StringError ) {
-Message = Error.getMessage();
-  });
-  EXPECT_EQ(Message, 

[PATCH] D37681: [refactor] Simplify the interface and remove some template magic

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

I will commit this today. @klimek, let me know if there any issues in 
post-commit review.


Repository:
  rL LLVM

https://reviews.llvm.org/D37681



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

[PATCH] D37681: [refactor] Simplify the interface and remove some template magic

[Eric Liu via Phabricator via cfe-commits]

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

@klimek Have you got a chance to take a look?


Repository:
  rL LLVM

https://reviews.llvm.org/D37681



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

[PATCH] D37681: [refactor] Simplify the interface and remove some template magic

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 115211.
arphaman marked 2 inline comments as done.
arphaman added a comment.

Make methods private


Repository:
  rL LLVM

https://reviews.llvm.org/D37681

Files:
  include/clang/Tooling/Refactoring/RefactoringActionRule.h
  include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  include/clang/Tooling/Refactoring/RefactoringActionRuleRequirementsInternal.h
  include/clang/Tooling/Refactoring/RefactoringActionRules.h
  include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
  include/clang/Tooling/Refactoring/RefactoringResultConsumer.h
  include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
  lib/Tooling/Refactoring/CMakeLists.txt
  lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  lib/Tooling/Refactoring/SourceSelectionConstraints.cpp
  unittests/Tooling/RefactoringActionRulesTest.cpp

Index: unittests/Tooling/RefactoringActionRulesTest.cpp
===
--- unittests/Tooling/RefactoringActionRulesTest.cpp
+++ unittests/Tooling/RefactoringActionRulesTest.cpp
@@ -18,7 +18,6 @@
 
 using namespace clang;
 using namespace tooling;
-using namespace refactoring_action_rules;
 
 namespace {
 
@@ -56,29 +55,39 @@
 }
 
 TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
-  auto ReplaceAWithB =
-  [](const RefactoringRuleContext &,
- std::pair Selection)
-  -> Expected {
-const SourceManager  = Selection.first.getSources();
-SourceLocation Loc = Selection.first.getRange().getBegin().getLocWithOffset(
-Selection.second);
-AtomicChange Change(SM, Loc);
-llvm::Error E = Change.replace(SM, Loc, 1, "b");
-if (E)
-  return std::move(E);
-return AtomicChanges{Change};
+  class ReplaceAWithB : public SourceChangeRefactoringRule {
+std::pair Selection;
+
+  public:
+ReplaceAWithB(std::pair Selection)
+: Selection(Selection) {}
+
+Expected
+createSourceReplacements(RefactoringRuleContext ) {
+  const SourceManager  = Context.getSources();
+  SourceLocation Loc =
+  Selection.first.getBegin().getLocWithOffset(Selection.second);
+  AtomicChange Change(SM, Loc);
+  llvm::Error E = Change.replace(SM, Loc, 1, "b");
+  if (E)
+return std::move(E);
+  return AtomicChanges{Change};
+}
   };
-  class SelectionRequirement : public selection::Requirement {
+
+  class SelectionRequirement : public SourceRangeSelectionRequirement {
   public:
-std::pair
-evaluateSelection(const RefactoringRuleContext &,
-  selection::SourceSelectionRange Selection) const {
-  return std::make_pair(Selection, 20);
+Expected>
+evaluate(RefactoringRuleContext ) const {
+  Expected R =
+  SourceRangeSelectionRequirement::evaluate(Context);
+  if (!R)
+return R.takeError();
+  return std::make_pair(*R, 20);
 }
   };
-  auto Rule = createRefactoringRule(ReplaceAWithB,
-requiredSelection(SelectionRequirement()));
+  auto Rule =
+  createRefactoringActionRule(SelectionRequirement());
 
   // When the requirements are satisifed, the rule's function must be invoked.
   {
@@ -123,54 +132,24 @@
 llvm::handleAllErrors(
 ErrorOrResult.takeError(),
 [&](llvm::StringError ) { Message = Error.getMessage(); });
-EXPECT_EQ(Message, "refactoring action can't be initiated with the "
-   "specified selection range");
+EXPECT_EQ(Message,
+  "refactoring action can't be initiated without a selection");
   }
 }
 
 TEST_F(RefactoringActionRulesTest, ReturnError) {
-  Expected (*Func)(const RefactoringRuleContext &,
-  selection::SourceSelectionRange) =
-  [](const RefactoringRuleContext &,
- selection::SourceSelectionRange) -> Expected {
-return llvm::make_error(
-"Error", llvm::make_error_code(llvm::errc::invalid_argument));
-  };
-  auto Rule = createRefactoringRule(
-  Func, requiredSelection(
-selection::identity()));
-
-  RefactoringRuleContext RefContext(Context.Sources);
-  SourceLocation Cursor =
-  Context.Sources.getLocForStartOfFile(Context.Sources.getMainFileID());
-  RefContext.setSelectionRange({Cursor, Cursor});
-  Expected Result = createReplacements(Rule, RefContext);
-
-  ASSERT_TRUE(!Result);
-  std::string Message;
-  llvm::handleAllErrors(Result.takeError(), [&](llvm::StringError ) {
-Message = Error.getMessage();
-  });
-  EXPECT_EQ(Message, "Error");
-}
-
-TEST_F(RefactoringActionRulesTest, ReturnInitiationDiagnostic) {
-  RefactoringRuleContext RefContext(Context.Sources);
-  class SelectionRequirement : public selection::Requirement {
+  class ErrorRule : public SourceChangeRefactoringRule {
   public:
-

[PATCH] D37681: [refactor] Simplify the interface and remove some template magic

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

This is very nice! Thanks!

Looks good to me; I'll let Manuel stamp the patch for you.




Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:56
+  virtual Expected
+  createSourceReplacements(RefactoringRuleContext ) = 0;
+

Can this be `private`?



Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:77
+  virtual Expected
+  findSymbolOccurrences(RefactoringRuleContext ) = 0;
 

`private`?


Repository:
  rL LLVM

https://reviews.llvm.org/D37681



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

[PATCH] D37681: [refactor] Simplify the interface and remove some template magic

[Alex Lorenz via Phabricator via cfe-commits]

arphaman created this revision.
Herald added a subscriber: mgorny.

This patch tries to simplify the interface for the refactoring action rules and 
refactoring requirements. It merges the selection constraints and selection 
requirements into one class. The refactoring actions rules must now be 
implemented using subclassing instead of raw function / lambda pointers. This 
allowed me to get rid of a bunch of traits templates and other template checks.


Repository:
  rL LLVM

https://reviews.llvm.org/D37681

Files:
  include/clang/Tooling/Refactoring/RefactoringActionRule.h
  include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  include/clang/Tooling/Refactoring/RefactoringActionRuleRequirementsInternal.h
  include/clang/Tooling/Refactoring/RefactoringActionRules.h
  include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
  include/clang/Tooling/Refactoring/RefactoringResultConsumer.h
  include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
  lib/Tooling/Refactoring/CMakeLists.txt
  lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  lib/Tooling/Refactoring/SourceSelectionConstraints.cpp
  unittests/Tooling/RefactoringActionRulesTest.cpp

Index: unittests/Tooling/RefactoringActionRulesTest.cpp
===
--- unittests/Tooling/RefactoringActionRulesTest.cpp
+++ unittests/Tooling/RefactoringActionRulesTest.cpp
@@ -18,7 +18,6 @@
 
 using namespace clang;
 using namespace tooling;
-using namespace refactoring_action_rules;
 
 namespace {
 
@@ -53,29 +52,39 @@
 }
 
 TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
-  auto ReplaceAWithB =
-  [](const RefactoringRuleContext &,
- std::pair Selection)
-  -> Expected {
-const SourceManager  = Selection.first.getSources();
-SourceLocation Loc = Selection.first.getRange().getBegin().getLocWithOffset(
-Selection.second);
-AtomicChange Change(SM, Loc);
-llvm::Error E = Change.replace(SM, Loc, 1, "b");
-if (E)
-  return std::move(E);
-return AtomicChanges{Change};
+  class ReplaceAWithB : public SourceChangeRefactoringRule {
+std::pair Selection;
+
+  public:
+ReplaceAWithB(std::pair Selection)
+: Selection(Selection) {}
+
+Expected
+createSourceReplacements(RefactoringRuleContext ) {
+  const SourceManager  = Context.getSources();
+  SourceLocation Loc =
+  Selection.first.getBegin().getLocWithOffset(Selection.second);
+  AtomicChange Change(SM, Loc);
+  llvm::Error E = Change.replace(SM, Loc, 1, "b");
+  if (E)
+return std::move(E);
+  return AtomicChanges{Change};
+}
   };
-  class SelectionRequirement : public selection::Requirement {
+
+  class SelectionRequirement : public SourceRangeSelectionRequirement {
   public:
-std::pair
-evaluateSelection(const RefactoringRuleContext &,
-  selection::SourceSelectionRange Selection) const {
-  return std::make_pair(Selection, 20);
+Expected>
+evaluate(RefactoringRuleContext ) const {
+  Expected R =
+  SourceRangeSelectionRequirement::evaluate(Context);
+  if (!R)
+return R.takeError();
+  return std::make_pair(*R, 20);
 }
   };
-  auto Rule = createRefactoringRule(ReplaceAWithB,
-requiredSelection(SelectionRequirement()));
+  auto Rule =
+  createRefactoringActionRule(SelectionRequirement());
 
   // When the requirements are satisifed, the rule's function must be invoked.
   {
@@ -120,54 +129,24 @@
 llvm::handleAllErrors(
 ErrorOrResult.takeError(),
 [&](llvm::StringError ) { Message = Error.getMessage(); });
-EXPECT_EQ(Message, "refactoring action can't be initiated with the "
-   "specified selection range");
+EXPECT_EQ(Message,
+  "refactoring action can't be initiated without a selection");
   }
 }
 
 TEST_F(RefactoringActionRulesTest, ReturnError) {
-  Expected (*Func)(const RefactoringRuleContext &,
-  selection::SourceSelectionRange) =
-  [](const RefactoringRuleContext &,
- selection::SourceSelectionRange) -> Expected {
-return llvm::make_error(
-"Error", llvm::make_error_code(llvm::errc::invalid_argument));
-  };
-  auto Rule = createRefactoringRule(
-  Func, requiredSelection(
-selection::identity()));
-
-  RefactoringRuleContext RefContext(Context.Sources);
-  SourceLocation Cursor =
-  Context.Sources.getLocForStartOfFile(Context.Sources.getMainFileID());
-  RefContext.setSelectionRange({Cursor, Cursor});
-  Expected Result = createReplacements(Rule, RefContext);
-
-  ASSERT_TRUE(!Result);
-  std::string Message;
-  llvm::handleAllErrors(Result.takeError(), [&](llvm::StringError ) {
-Message =