[PATCH] D36075: [refactor] Initial support for refactoring action rules
klimek added inline comments.
Comment at:
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h:23-37
+/// A common refactoring action rule interface.
+class RefactoringActionRule {
+public:
+ enum RuleKind { SourceChangeRefactoringRuleKind };
+
+ RuleKind getRuleKind() const { return Kind; }
+
arphaman wrote:
> klimek wrote:
> > Sorry for being late, was out on vacation.
> > Generally, why do we need this tag-based abstraction here instead of using
> > the more typical OO double-dispatch where necessary?
> > (We do this in the AST a lot, but the AST is special, because there we want
> > to implement a lot of different algorithms that rely on the node type,
> > while I don't see how that applies here)
> Generally the clients will have to somehow distinguish between the types of
> results that are produced by rules to figure out what to do (e.g.
> AtomicChanges -> apply, SymbolOccurrences -> ask user, Continuation -> look
> for more ASTs). So far I've thought that the LLVM based dynamic casts will
> work well for this, e.g.
>
> ```
> if (auto *Action = dyn_cast()) {
> Expected Changes =
> Action->createSourceReplacements();
> applyChanges(Changes);
> } else if (...) {
> ...
> } else (...) {
>...
> }
> ```
>
> But you're probably right, there might be a better way to do this rather than
> the tag based approach. Something like a consumer class that clients can
> implement that provides consumer functions that take in the specific results.
> I reckon a single consumer will actually work better in the long-run when we
> might Continuations that both return changes in the first TU and information
> for searches in other TUs. I'll see if I can get a patch out that removes
> this tag and uses the consumer approach.
Cool, looking forward to it :)
Repository:
rL LLVM
https://reviews.llvm.org/D36075
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman added inline comments.
Comment at:
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h:23-37
+/// A common refactoring action rule interface.
+class RefactoringActionRule {
+public:
+ enum RuleKind { SourceChangeRefactoringRuleKind };
+
+ RuleKind getRuleKind() const { return Kind; }
+
klimek wrote:
> Sorry for being late, was out on vacation.
> Generally, why do we need this tag-based abstraction here instead of using
> the more typical OO double-dispatch where necessary?
> (We do this in the AST a lot, but the AST is special, because there we want
> to implement a lot of different algorithms that rely on the node type, while
> I don't see how that applies here)
Generally the clients will have to somehow distinguish between the types of
results that are produced by rules to figure out what to do (e.g. AtomicChanges
-> apply, SymbolOccurrences -> ask user, Continuation -> look for more ASTs).
So far I've thought that the LLVM based dynamic casts will work well for this,
e.g.
```
if (auto *Action = dyn_cast()) {
Expected Changes =
Action->createSourceReplacements();
applyChanges(Changes);
} else if (...) {
...
} else (...) {
...
}
```
But you're probably right, there might be a better way to do this rather than
the tag based approach. Something like a consumer class that clients can
implement that provides consumer functions that take in the specific results. I
reckon a single consumer will actually work better in the long-run when we
might Continuations that both return changes in the first TU and information
for searches in other TUs. I'll see if I can get a patch out that removes this
tag and uses the consumer approach.
Repository:
rL LLVM
https://reviews.llvm.org/D36075
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
klimek added inline comments.
Comment at:
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h:23-37
+/// A common refactoring action rule interface.
+class RefactoringActionRule {
+public:
+ enum RuleKind { SourceChangeRefactoringRuleKind };
+
+ RuleKind getRuleKind() const { return Kind; }
+
Sorry for being late, was out on vacation.
Generally, why do we need this tag-based abstraction here instead of using the
more typical OO double-dispatch where necessary?
(We do this in the AST a lot, but the AST is special, because there we want to
implement a lot of different algorithms that rely on the node type, while I
don't see how that applies here)
Repository:
rL LLVM
https://reviews.llvm.org/D36075
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
This revision was automatically updated to reflect the committed changes.
Closed by commit rL311884: [refactor] initial support for refactoring action
rules (authored by arphaman).
Changed prior to commit:
https://reviews.llvm.org/D36075?vs=112678=112879#toc
Repository:
rL LLVM
https://reviews.llvm.org/D36075
Files:
cfe/trunk/include/clang/Basic/LLVM.h
cfe/trunk/include/clang/Tooling/Refactoring/AtomicChange.h
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/RefactoringRuleContext.h
cfe/trunk/include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
cfe/trunk/lib/Tooling/Refactoring/SourceSelectionConstraints.cpp
cfe/trunk/unittests/Tooling/CMakeLists.txt
cfe/trunk/unittests/Tooling/RefactoringActionRulesTest.cpp
Index: cfe/trunk/lib/Tooling/Refactoring/SourceSelectionConstraints.cpp
===
--- cfe/trunk/lib/Tooling/Refactoring/SourceSelectionConstraints.cpp
+++ cfe/trunk/lib/Tooling/Refactoring/SourceSelectionConstraints.cpp
@@ -0,0 +1,23 @@
+//===--- SourceSelectionConstraints.cpp - Evaluate selection constraints --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/SourceSelectionConstraints.h"
+#include "clang/Tooling/Refactoring/RefactoringRuleContext.h"
+
+using namespace clang;
+using namespace tooling;
+using namespace selection;
+
+Optional
+SourceSelectionRange::evaluate(RefactoringRuleContext ) {
+ SourceRange R = Context.getSelectionRange();
+ if (R.isInvalid())
+return None;
+ return SourceSelectionRange(Context.getSources(), R);
+}
Index: cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
===
--- cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
+++ cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
@@ -8,6 +8,7 @@
Rename/USRFinder.cpp
Rename/USRFindingAction.cpp
Rename/USRLocFinder.cpp
+ SourceSelectionConstraints.cpp
LINK_LIBS
clangAST
Index: cfe/trunk/unittests/Tooling/CMakeLists.txt
===
--- cfe/trunk/unittests/Tooling/CMakeLists.txt
+++ cfe/trunk/unittests/Tooling/CMakeLists.txt
@@ -25,6 +25,7 @@
RecursiveASTVisitorTestDeclVisitor.cpp
RecursiveASTVisitorTestExprVisitor.cpp
RecursiveASTVisitorTestTypeLocVisitor.cpp
+ RefactoringActionRulesTest.cpp
RefactoringCallbacksTest.cpp
RefactoringTest.cpp
ReplacementsYamlTest.cpp
Index: cfe/trunk/unittests/Tooling/RefactoringActionRulesTest.cpp
===
--- cfe/trunk/unittests/Tooling/RefactoringActionRulesTest.cpp
+++ cfe/trunk/unittests/Tooling/RefactoringActionRulesTest.cpp
@@ -0,0 +1,165 @@
+//===- unittest/Tooling/RefactoringTestActionRulesTest.cpp ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ReplacementTest.h"
+#include "RewriterTestContext.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/RefactoringActionRules.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Support/Errc.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace tooling;
+using namespace refactoring_action_rules;
+
+namespace {
+
+class RefactoringActionRulesTest : public ::testing::Test {
+protected:
+ void SetUp() override {
+Context.Sources.setMainFileID(
+Context.createInMemoryFile("input.cpp", DefaultCode));
+ }
+
+ RewriterTestContext Context;
+ std::string DefaultCode = std::string(100, 'a');
+};
+
+Expected
+createReplacements(const std::unique_ptr ,
+ RefactoringRuleContext ) {
+ return cast(*Rule).createSourceReplacements(
+ Context);
+}
+
+TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
+ auto ReplaceAWithB =
+ [](std::pair Selection)
+ -> Expected {
+const SourceManager = Selection.first.getSources();
+SourceLocation Loc = Selection.first.getRange().getBegin().getLocWithOffset(
+Selection.second);
+AtomicChange Change(SM, Loc);
+
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman added a comment. Thanks, I'll start working on the documentation patch and will split follow-up `clang-refactor` patch into one or two parts today. Comment at: include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:33 +public: + virtual Expected+ perform(RefactoringRuleContext ) = 0; ioeric wrote: > Why isn't this a interface in `SpecificRefactoringRuleAdapter` with return > type `Expected `? A method declaration in `SpecificRefactoringRuleAdapter` won't work since it won't be available in either the template specialisation or the deriving class as the classes won't be directly related. I could use a separate parent class independent of SpecificRefactoringRuleAdapter that declares a generic interface though. Repository: rL LLVM https://reviews.llvm.org/D36075 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. Thanks for the changes! Lgtm with a few nits. Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:33 +/// +/// - requiredSelection: The refactoring function won't be invoked unless the +/// given selection requirement is satisfied. arphaman wrote: > ioeric wrote: > > We might want to document supported requirements somewhere else so that we > > don't need to update this file every time a new requirement is added. > Do you think it should be in Clang's documentation? I can start on a new > document there but I'd prefer to do it in a separate patch. WDYT? Sure, this is fine for now. It would be nice to have proper documentation in the future when pieces get into places. Comment at: include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:33 +public: + virtual Expected+ perform(RefactoringRuleContext ) = 0; Why isn't this a interface in `SpecificRefactoringRuleAdapter` with return type `Expected `? Comment at: unittests/Tooling/RefactoringActionRulesTest.cpp:67 + { +RefactoringRuleContext Operation(Context.Sources); +SourceLocation Cursor = It would be nice to also rename the variable from `Operation` to `Context`. Repository: rL LLVM https://reviews.llvm.org/D36075 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman updated this revision to Diff 112678.
arphaman added a comment.
- Get rid of `RefactoringResult` in favour of different rule types.
- Rename `RefactoringOperation` to `RefactoringRuleContext`.
- Address a couple more comments.
Repository:
rL LLVM
https://reviews.llvm.org/D36075
Files:
include/clang/Basic/LLVM.h
include/clang/Tooling/Refactoring/AtomicChange.h
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/RefactoringRuleContext.h
include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
lib/Tooling/Refactoring/CMakeLists.txt
lib/Tooling/Refactoring/SourceSelectionConstraints.cpp
unittests/Tooling/CMakeLists.txt
unittests/Tooling/RefactoringActionRulesTest.cpp
Index: unittests/Tooling/RefactoringActionRulesTest.cpp
===
--- /dev/null
+++ unittests/Tooling/RefactoringActionRulesTest.cpp
@@ -0,0 +1,165 @@
+//===- unittest/Tooling/RefactoringTestActionRulesTest.cpp ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ReplacementTest.h"
+#include "RewriterTestContext.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/RefactoringActionRules.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Support/Errc.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace tooling;
+using namespace refactoring_action_rules;
+
+namespace {
+
+class RefactoringActionRulesTest : public ::testing::Test {
+protected:
+ void SetUp() override {
+Context.Sources.setMainFileID(
+Context.createInMemoryFile("input.cpp", DefaultCode));
+ }
+
+ RewriterTestContext Context;
+ std::string DefaultCode = std::string(100, 'a');
+};
+
+Expected
+createReplacements(const std::unique_ptr ,
+ RefactoringRuleContext ) {
+ return cast(*Rule).createSourceReplacements(
+ Context);
+}
+
+TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
+ auto ReplaceAWithB =
+ [](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(selection::SourceSelectionRange Selection) const {
+ return std::make_pair(Selection, 20);
+}
+ };
+ auto Rule = createRefactoringRule(ReplaceAWithB,
+requiredSelection(SelectionRequirement()));
+
+ // When the requirements are satisifed, the rule's function must be invoked.
+ {
+RefactoringRuleContext Operation(Context.Sources);
+SourceLocation Cursor =
+Context.Sources.getLocForStartOfFile(Context.Sources.getMainFileID())
+.getLocWithOffset(10);
+Operation.setSelectionRange({Cursor, Cursor});
+
+Expected ErrorOrResult =
+createReplacements(Rule, Operation);
+ASSERT_FALSE(!ErrorOrResult);
+ASSERT_FALSE(!*ErrorOrResult);
+AtomicChanges Result = std::move(**ErrorOrResult);
+ASSERT_EQ(Result.size(), 1u);
+std::string YAMLString =
+const_cast(Result[0]).toYAMLString();
+
+ASSERT_STREQ("---\n"
+ "Key: 'input.cpp:30'\n"
+ "FilePath:input.cpp\n"
+ "Error: ''\n"
+ "InsertedHeaders: \n"
+ "RemovedHeaders: \n"
+ "Replacements:\n" // Extra whitespace here!
+ " - FilePath:input.cpp\n"
+ "Offset: 30\n"
+ "Length: 1\n"
+ "ReplacementText: b\n"
+ "...\n",
+ YAMLString.c_str());
+ }
+
+ // When one of the requirements is not satisfied, perform should return either
+ // None or a valid diagnostic.
+ {
+RefactoringRuleContext Operation(Context.Sources);
+Expected ErrorOrResult =
+createReplacements(Rule, Operation);
+
+ASSERT_FALSE(!ErrorOrResult);
+Optional Value = std::move(*ErrorOrResult);
+EXPECT_TRUE(!Value);
+ }
+}
+
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman marked 5 inline comments as done. arphaman added inline comments. Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:33 +/// +/// - requiredSelection: The refactoring function won't be invoked unless the +/// given selection requirement is satisfied. ioeric wrote: > We might want to document supported requirements somewhere else so that we > don't need to update this file every time a new requirement is added. Do you think it should be in Clang's documentation? I can start on a new document there but I'd prefer to do it in a separate patch. WDYT? Repository: rL LLVM https://reviews.llvm.org/D36075 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
alexshap added a comment. @arphaman The selection requirement is supposed to be orthogonal to AST matchers, not a replacement. It should be used when working with source selection in editors. I did mess around with moving over clang-reorder-fields using this kind of model and didn't see any issues when using AST matchers. Essentially I used the requiredOption requirements and mapped them to run my matching code instead of using the selection requirement. Thus this requirement was satisfied only when the AST matchers were successful. It might be possible to simplify that pattern even further to make it simpler. that's great, i'm interested in this too and would be happy to see clang-reorder-fields moving to clang-refactor (pls, let me know if i can help make this happen) Repository: rL LLVM https://reviews.llvm.org/D36075 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman added inline comments. Comment at: lib/Tooling/Refactoring/SourceSelectionConstraints.cpp:13 + +using namespace clang; +using namespace tooling; ioeric wrote: > We are tempted to avoid `using namespace` if possible. Why? It's not in a header. `using namespace clang` is the common practice across all of Clang's sources. Repository: rL LLVM https://reviews.llvm.org/D36075 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman added inline comments.
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:21
+struct RefactoringResult {
+ enum ResultKind {
+/// A set of source replacements represented using a vector of
ioeric wrote:
> arphaman wrote:
> > ioeric wrote:
> > > I'm a bit unsure about the abstraction of the refactoring result. I would
> > > expected refactoring results to be source changes always. Do you have any
> > > refactoring tool that outputs occurrences in mind?
> > In Xcode we require rename to return symbol occurrences because the IDE is
> > responsible for figuring out:
> > 1) The new name. Thus we can't produce replacements when renaming because
> > we don't know what the new name is when gathering the occurrences.
> > 2) Whether these occurrences should be actually applied. Thus we can't
> > produce replacements because it's up to the user to decide if they want to
> > rename some occurrence in a comment for example.
> >
> > In general 2) can be applied to tools like clang-refactor that could allow
> > users to select occurrences that don't guarantee a direct semantic match
> > (comments, etc.) in an interactive manner.
> >
> > I think clangd has a rather naive rename support, so these points may not
> > apply, but we may want to extend clangd's support for rename in the future
> > as well.
> I feel occurrences are more of an intermediate state of a refactoring action
> than a result. I'm wondering if it makes sense to introduce a separate class
> to represent such intermediate states? I am a bit nervous to fuse multiple
> classes into one; the interfaces can get pretty ugly when more result kinds
> are added.
Good point. I agree.
I think it would be better to differentiate between `RefactoringActionRules`
then. Ordinary rules return a set of AtomicChanges instead of
RefactoringResult. But then we could also have "interactive" rules that return
"partial" results like symbol occurrences.
I think I'll try the following approach:
```
class RefactoringActionRule {
virtual ~RefactoringActionRule() {}
};
class RefactoringActionSourceChangeRule: public RefactoringActionRule {
public:
virtual Expected
createSourceReplacements(RefactoringOperation ) = 0;
};
class RefactoringActionSymbolOccurrencesRule: public RefactoringActionRule {
public:
virtual Expected
findSymbolOccurrences(RefactoringOperation ) = 0;
};
```
Repository:
rL LLVM
https://reviews.llvm.org/D36075
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
ioeric added inline comments.
Comment at:
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:26
+template
+detail::SourceSelectionRequirement<
+typename selection::detail::EvaluateSelectionChecker<
arphaman wrote:
> ioeric wrote:
> > Could you help me understand this class?
> >
> > This seems to be a selection-specific requirement and should live in
> > `selection`. It is also used in `BaseSpecializedRule` which seems to be a
> > higher level of abstraction.
> It's just a container class that stores all information about the
> `requiredSelection` requirement. I agree about `BaseSpecializedRule`, that
> connection should be chopped. I will move the evaluation code into the
> requirement itself when I update the patch.
>
> I would tend to disagree about moving it though, as
> `SourceSelectionRequirement` is a requirement first and I think that's why it
> should live with other requirements. Yes, it's related to selections, but it
> uses them to implement the requirement. I think it's better to keep
> requirements together, as opposed to having `option` requirements close to
> options, selection requirements close to selection, and so on. WDYT?
Thanks!
Makes sense. We might want to put individual requirements into their own
headers so that this doesn't grow into a huge file when more requirements are
supported.
Comment at:
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirementsInternal.h:42
+ RequirementBase>::type {
+ using OutputType = typename DropExpectedOptional::Type;
+
It might worth having a comment explaining why and how `Expected` is
wrapped and unwrapped during the evaluation.
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:33
+///
+/// - requiredSelection: The refactoring function won't be invoked unless the
+/// given selection requirement is satisfied.
We might want to document supported requirements somewhere else so that we
don't need to update this file every time a new requirement is added.
Comment at: include/clang/Tooling/Refactoring/RefactoringOperation.h:1
+//===--- RefactoringOperationController.h - Clang refactoring library
-===//
+//
s/RefactoringOperationController.h/RefactoringOperation.h/ :)
Comment at: include/clang/Tooling/Refactoring/RefactoringOperation.h:29
+/// to represent a selection in an editor.
+class RefactoringOperation {
+public:
I found the name a bit confusing - `RefactoringOperation` sounds a bit like
`RefactoringAction`.
Would it make sense to call this `RefactoringContext` or
`RefactoringRuleContext`, if this stores states of a refactoring rule?
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:21
+struct RefactoringResult {
+ enum ResultKind {
+/// A set of source replacements represented using a vector of
arphaman wrote:
> ioeric wrote:
> > I'm a bit unsure about the abstraction of the refactoring result. I would
> > expected refactoring results to be source changes always. Do you have any
> > refactoring tool that outputs occurrences in mind?
> In Xcode we require rename to return symbol occurrences because the IDE is
> responsible for figuring out:
> 1) The new name. Thus we can't produce replacements when renaming because we
> don't know what the new name is when gathering the occurrences.
> 2) Whether these occurrences should be actually applied. Thus we can't
> produce replacements because it's up to the user to decide if they want to
> rename some occurrence in a comment for example.
>
> In general 2) can be applied to tools like clang-refactor that could allow
> users to select occurrences that don't guarantee a direct semantic match
> (comments, etc.) in an interactive manner.
>
> I think clangd has a rather naive rename support, so these points may not
> apply, but we may want to extend clangd's support for rename in the future as
> well.
I feel occurrences are more of an intermediate state of a refactoring action
than a result. I'm wondering if it makes sense to introduce a separate class to
represent such intermediate states? I am a bit nervous to fuse multiple classes
into one; the interfaces can get pretty ugly when more result kinds are added.
Comment at: lib/Tooling/Refactoring/SourceSelectionConstraints.cpp:13
+
+using namespace clang;
+using namespace tooling;
We are tempted to avoid `using namespace` if possible.
Repository:
rL LLVM
https://reviews.llvm.org/D36075
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman updated this revision to Diff 112566.
arphaman marked 2 inline comments as done.
arphaman added a comment.
- Rename `detail` to `internal`.
- Remove the `BaseSpecializedRule`.
- Simplify selection requirement and constraint evaluation.
Repository:
rL LLVM
https://reviews.llvm.org/D36075
Files:
include/clang/Basic/LLVM.h
include/clang/Tooling/Refactoring/AtomicChange.h
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/RefactoringOperation.h
include/clang/Tooling/Refactoring/RefactoringResult.h
include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
lib/Tooling/Refactoring/CMakeLists.txt
lib/Tooling/Refactoring/SourceSelectionConstraints.cpp
unittests/Tooling/CMakeLists.txt
unittests/Tooling/RefactoringActionRulesTest.cpp
Index: unittests/Tooling/RefactoringActionRulesTest.cpp
===
--- /dev/null
+++ unittests/Tooling/RefactoringActionRulesTest.cpp
@@ -0,0 +1,157 @@
+//===- unittest/Tooling/RefactoringTestActionRulesTest.cpp ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ReplacementTest.h"
+#include "RewriterTestContext.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/RefactoringActionRules.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Support/Errc.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace tooling;
+using namespace refactoring_action_rules;
+
+namespace {
+
+class RefactoringActionRulesTest : public ::testing::Test {
+protected:
+ void SetUp() override {
+Context.Sources.setMainFileID(
+Context.createInMemoryFile("input.cpp", DefaultCode));
+ }
+
+ RewriterTestContext Context;
+ std::string DefaultCode = std::string(100, 'a');
+};
+
+TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
+ auto ReplaceAWithB =
+ [](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 RefactoringResult(Change);
+ };
+ class SelectionRequirement : public selection::Requirement {
+ public:
+std::pair
+evaluateSelection(selection::SourceSelectionRange Selection) const {
+ return std::make_pair(Selection, 20);
+}
+ };
+ auto Rule = createRefactoringRule(ReplaceAWithB,
+requiredSelection(SelectionRequirement()));
+
+ // When the requirements are satisifed, the rule's function must be invoked.
+ {
+RefactoringOperation Operation(Context.Sources);
+SourceLocation Cursor =
+Context.Sources.getLocForStartOfFile(Context.Sources.getMainFileID())
+.getLocWithOffset(10);
+Operation.setSelectionRange({Cursor, Cursor});
+
+Expected ErrorOrResult =
+Rule->perform(Operation);
+ASSERT_FALSE(!ErrorOrResult);
+ASSERT_FALSE(!*ErrorOrResult);
+RefactoringResult Result = std::move(**ErrorOrResult);
+ASSERT_EQ(Result.getKind(), RefactoringResult::AtomicChanges);
+ASSERT_EQ(Result.getChanges().size(), 1u);
+std::string YAMLString =
+const_cast(Result.getChanges()[0]).toYAMLString();
+
+ASSERT_STREQ("---\n"
+ "Key: 'input.cpp:30'\n"
+ "FilePath:input.cpp\n"
+ "Error: ''\n"
+ "InsertedHeaders: \n"
+ "RemovedHeaders: \n"
+ "Replacements:\n" // Extra whitespace here!
+ " - FilePath:input.cpp\n"
+ "Offset: 30\n"
+ "Length: 1\n"
+ "ReplacementText: b\n"
+ "...\n",
+ YAMLString.c_str());
+ }
+
+ // When one of the requirements is not satisfied, perform should return either
+ // None or a valid diagnostic.
+ {
+RefactoringOperation Operation(Context.Sources);
+Expected ErrorOrResult =
+Rule->perform(Operation);
+
+ASSERT_FALSE(!ErrorOrResult);
+Optional Value = std::move(*ErrorOrResult);
+EXPECT_TRUE(!Value);
+ }
+}
+
+TEST_F(RefactoringActionRulesTest, ReturnError) {
+
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman added inline comments. Comment at: include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:26 +template +detail::SourceSelectionRequirement< +typename selection::detail::EvaluateSelectionChecker< ioeric wrote: > Could you help me understand this class? > > This seems to be a selection-specific requirement and should live in > `selection`. It is also used in `BaseSpecializedRule` which seems to be a > higher level of abstraction. It's just a container class that stores all information about the `requiredSelection` requirement. I agree about `BaseSpecializedRule`, that connection should be chopped. I will move the evaluation code into the requirement itself when I update the patch. I would tend to disagree about moving it though, as `SourceSelectionRequirement` is a requirement first and I think that's why it should live with other requirements. Yes, it's related to selections, but it uses them to implement the requirement. I think it's better to keep requirements together, as opposed to having `option` requirements close to options, selection requirements close to selection, and so on. WDYT? Repository: rL LLVM https://reviews.llvm.org/D36075 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman added a comment. In https://reviews.llvm.org/D36075#851278, @ioeric wrote: > Thanks for the changes! The code is much clearer. > > I am wondering if the current design could be extended to support tools (or > rules) that use AST matchers? Or is the selection expected to be powerful > enough to replace AST matchers? > > We have a few tools in `clang-tools-extra` (e.g. `clang-move` and > `change-namespace`) and many internal tools that are based on AST matchers, > but we would really like to move them into `clang-refactor` in the future :) The selection requirement is supposed to be orthogonal to AST matchers, not a replacement. It should be used when working with source selection in editors. I did mess around with moving over `clang-reorder-fields` using this kind of model and didn't see any issues when using AST matchers. Essentially I used the `requiredOption` requirements and mapped them to run my matching code instead of using the selection requirement. Thus this requirement was satisfied only when the AST matchers were successful. It might be possible to simplify that pattern even further to make it simpler. In https://reviews.llvm.org/D36075#851278, @ioeric wrote: > Thanks for the changes! The code is much clearer. > > I am wondering if the current design could be extended to support tools (or > rules) that use AST matchers? Or is the selection expected to be powerful > enough to replace AST matchers? > > We have a few tools in `clang-tools-extra` (e.g. `clang-move` and > `change-namespace`) and many internal tools that are based on AST matchers, > but we would really like to move them into `clang-refactor` in the future :) Repository: rL LLVM https://reviews.llvm.org/D36075 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman added inline comments.
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:21
+struct RefactoringResult {
+ enum ResultKind {
+/// A set of source replacements represented using a vector of
ioeric wrote:
> I'm a bit unsure about the abstraction of the refactoring result. I would
> expected refactoring results to be source changes always. Do you have any
> refactoring tool that outputs occurrences in mind?
In Xcode we require rename to return symbol occurrences because the IDE is
responsible for figuring out:
1) The new name. Thus we can't produce replacements when renaming because we
don't know what the new name is when gathering the occurrences.
2) Whether these occurrences should be actually applied. Thus we can't produce
replacements because it's up to the user to decide if they want to rename some
occurrence in a comment for example.
In general 2) can be applied to tools like clang-refactor that could allow
users to select occurrences that don't guarantee a direct semantic match
(comments, etc.) in an interactive manner.
I think clangd has a rather naive rename support, so these points may not
apply, but we may want to extend clangd's support for rename in the future as
well.
Repository:
rL LLVM
https://reviews.llvm.org/D36075
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
ioeric added a comment.
Thanks for the changes! The code is much clearer.
I am wondering if the current design could be extended to support tools (or
rules) that use AST matchers? Or is the selection expected to be powerful
enough to replace AST matchers?
We have a few tools in `clang-tools-extra` (e.g. `clang-move` and
`change-namespace`) and many internal tools that are based on AST matchers, but
we would really like to move them into `clang-refactor` in the future :)
Comment at:
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:26
+template
+detail::SourceSelectionRequirement<
+typename selection::detail::EvaluateSelectionChecker<
Could you help me understand this class?
This seems to be a selection-specific requirement and should live in
`selection`. It is also used in `BaseSpecializedRule` which seems to be a
higher level of abstraction.
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:27
+
+ RefactoringResult(AtomicChange Change) : Kind(AtomicChanges) {
+Changes.push_back(std::move(Change));
arphaman wrote:
> alexshap wrote:
> > explicit ?
> Nah, it's more convenient to be able to return a single `AtomicChanges`
> without an explicit initializer I think.
+1 to `explicit` which could prevent unintentional conversion.
`RefactoringResult(Change);` isn't too bad IMO.
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:21
+struct RefactoringResult {
+ enum ResultKind {
+/// A set of source replacements represented using a vector of
I'm a bit unsure about the abstraction of the refactoring result. I would
expected refactoring results to be source changes always. Do you have any
refactoring tool that outputs occurrences in mind?
Comment at: include/clang/Tooling/Refactoring/SourceSelectionConstraints.h:55
+
+namespace detail {
+
Does `detail` mean internal implementation? Maybe use `internal` which is more
commonly used for this?
Repository:
rL LLVM
https://reviews.llvm.org/D36075
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman updated this revision to Diff 112195.
arphaman marked 7 inline comments as done.
arphaman added a comment.
- Split the header
- Remove DiagnosticOr in favour of Expected that will use a DiagnosticError
that was proposed in the other patch.
- Address the other review comments
Repository:
rL LLVM
https://reviews.llvm.org/D36075
Files:
include/clang/Basic/LLVM.h
include/clang/Tooling/Refactoring/AtomicChange.h
include/clang/Tooling/Refactoring/EvaluateSourceSelectionConstraints.h
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/RefactoringOperation.h
include/clang/Tooling/Refactoring/RefactoringResult.h
include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
unittests/Tooling/CMakeLists.txt
unittests/Tooling/RefactoringActionRulesTest.cpp
Index: unittests/Tooling/RefactoringActionRulesTest.cpp
===
--- /dev/null
+++ unittests/Tooling/RefactoringActionRulesTest.cpp
@@ -0,0 +1,157 @@
+//===- unittest/Tooling/RefactoringTestActionRulesTest.cpp ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ReplacementTest.h"
+#include "RewriterTestContext.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/RefactoringActionRules.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Support/Errc.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace tooling;
+using namespace refactoring_action_rules;
+
+namespace {
+
+class RefactoringActionRulesTest : public ::testing::Test {
+protected:
+ void SetUp() override {
+Context.Sources.setMainFileID(
+Context.createInMemoryFile("input.cpp", DefaultCode));
+ }
+
+ RewriterTestContext Context;
+ std::string DefaultCode = std::string(100, 'a');
+};
+
+TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
+ auto ReplaceAWithB =
+ [](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 Change;
+ };
+ class SelectionRequirement : public selection::Requirement {
+ public:
+std::pair
+evaluateSelection(selection::SourceSelectionRange Selection) const {
+ return std::make_pair(Selection, 20);
+}
+ };
+ auto Rule = createRefactoringRule(ReplaceAWithB,
+requiredSelection(SelectionRequirement()));
+
+ // When the requirements are satisifed, the rule's function must be invoked.
+ {
+RefactoringOperation Operation(Context.Sources);
+SourceLocation Cursor =
+Context.Sources.getLocForStartOfFile(Context.Sources.getMainFileID())
+.getLocWithOffset(10);
+Operation.setSelectionRange({Cursor, Cursor});
+
+Expected ErrorOrResult =
+Rule->perform(Operation);
+ASSERT_FALSE(!ErrorOrResult);
+ASSERT_FALSE(!*ErrorOrResult);
+RefactoringResult Result = std::move(**ErrorOrResult);
+ASSERT_EQ(Result.getKind(), RefactoringResult::AtomicChanges);
+ASSERT_EQ(Result.getChanges().size(), 1u);
+std::string YAMLString =
+const_cast(Result.getChanges()[0]).toYAMLString();
+
+ASSERT_STREQ("---\n"
+ "Key: 'input.cpp:30'\n"
+ "FilePath:input.cpp\n"
+ "Error: ''\n"
+ "InsertedHeaders: \n"
+ "RemovedHeaders: \n"
+ "Replacements:\n" // Extra whitespace here!
+ " - FilePath:input.cpp\n"
+ "Offset: 30\n"
+ "Length: 1\n"
+ "ReplacementText: b\n"
+ "...\n",
+ YAMLString.c_str());
+ }
+
+ // When one of the requirements is not satisfied, perform should return either
+ // None or a valid diagnostic.
+ {
+RefactoringOperation Operation(Context.Sources);
+Expected ErrorOrResult =
+Rule->perform(Operation);
+
+ASSERT_FALSE(!ErrorOrResult);
+Optional Value = std::move(*ErrorOrResult);
+EXPECT_TRUE(!Value);
+ }
+}
+
+TEST_F(RefactoringActionRulesTest, ReturnError) {
+
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman added inline comments.
Comment at:
include/clang/Tooling/Refactoring/RefactoringOperationController.h:19
+
+/// Encapsulates all of the possible state that an individual refactoring
+/// operation might have. Controls the process of initiation of refactoring
ioeric wrote:
> What are all the possible states, for example?
Bad comment. I think `inputs` is more descriptive.
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:27
+
+ RefactoringResult(AtomicChange Change) : Kind(AtomicChanges) {
+Changes.push_back(std::move(Change));
alexshap wrote:
> explicit ?
Nah, it's more convenient to be able to return a single `AtomicChanges` without
an explicit initializer I think.
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:35
+
+ llvm::MutableArrayRef getChanges() {
+assert(getKind() == AtomicChanges &&
ioeric wrote:
> Do we expect the result changes to be modified? Why?
No, that was a workaround for a non-const member use. I'll use a const cast
instead.
Comment at: include/clang/Tooling/Refactoring/SourceSelectionConstraints.h:35
+/// A custom selection requirement.
+class Requirement {
+ /// Subclasses must implement 'T evaluateSelection(SelectionConstraint)
const'
ioeric wrote:
> It might worth explaining the relationship between this and the
> `RequirementBase`.
This class is not related to `RequirementBase` though. Were you talking about
another class?
Repository:
rL LLVM
https://reviews.llvm.org/D36075
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman added a comment. Extracted `DiagnosticOr` to a separate patch at https://reviews.llvm.org/D36969. I will update this patch tomorrow. Repository: rL LLVM https://reviews.llvm.org/D36075 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
alexshap added inline comments.
Comment at: include/clang/Basic/DiagnosticOr.h:40
+ /// diagnostic.
+ DiagnosticOr(PartialDiagnosticAt Diagnostic) : HasDiagnostic(true) {
+new (getDiagnosticStorage()) PartialDiagnosticAt(std::move(Diagnostic));
explicit ?
Comment at: include/clang/Basic/DiagnosticOr.h:49
+ OtherT &,
+ typename std::enable_if::value>::type * =
+ nullptr)
but probably it would be a bit cleaner to enable SFINAE via a template
parameter (http://en.cppreference.com/w/cpp/types/enable_if , option #4) rather
than via extra argument.
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:27
+
+ RefactoringResult(AtomicChange Change) : Kind(AtomicChanges) {
+Changes.push_back(std::move(Change));
explicit ?
Repository:
rL LLVM
https://reviews.llvm.org/D36075
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
ioeric added a comment.
This is great work and definitely a lot to digest! ;) Some high-level comments
for the first round.
In general, I really appreciate the high-level interfaces; I am just a bit
concerned about the implementation which is a bit hard to follow at this point,
especially with all the template magics. I left some suggestions in the
comments; hopefully they would help.
I would also appreciate more detailed high-level comments on the major classes
and their relationships - I found myself constantly going back to the RFC to
understand their intentions.
Comment at: include/clang/Basic/DiagnosticOr.h:1
+//===--- DiagnosticOr.h - Diagnostic "closures" -*- C++
-*-===//
+//
Could you please split changes related to `DiagnosticOr` into a separate patch
and have owners of clang/Basic/ directory review it? ;)
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:1
+//===--- RefactoringActionRules.h - Clang refactoring library
-===//
+//
Code in this file is a bit hard to follow, especially with so many classes and
template magics :P
Is it possible to split them into smaller modules, and ideally with more
detailed documentation for each?
It seems to me that the following logics could live in their own headers:
- `RefactoringActionRule` interface.
- Base classes for requirements.
- SourceSlection-related requirements.
- Derived/specialized rules.
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:227
+std::unique_ptr
+apply(Expected (*RefactoringFunction)(
+ typename RequirementTypes::OutputType...),
Why is this called `apply`? I feel something like `createAction` or
`generateAction` would be more intuitive.
Comment at:
include/clang/Tooling/Refactoring/RefactoringOperationController.h:19
+
+/// Encapsulates all of the possible state that an individual refactoring
+/// operation might have. Controls the process of initiation of refactoring
What are all the possible states, for example?
Comment at:
include/clang/Tooling/Refactoring/RefactoringOperationController.h:22
+/// operations, by feeding the right information to the functions that
+/// evaluate the refactoring action rule requirements.
+class RefactoringOperationController {
Maybe I am missing too much context, but I found it hard to understand the
comment and couldn't relate the comment to the public interfaces. Could you
elaborate more?
It is also unclear to me if this is specific to the source selection.
Comment at:
include/clang/Tooling/Refactoring/RefactoringOperationController.h:40
+private:
+ const SourceManager
+ SourceRange SelectionRange;
It's unclear to me what the intentions of these members are. Could you add some
comments for these members?
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:35
+
+ llvm::MutableArrayRef getChanges() {
+assert(getKind() == AtomicChanges &&
Do we expect the result changes to be modified? Why?
Comment at: include/clang/Tooling/Refactoring/SourceSelectionConstraints.h:21
+/// This constraint is satisfied when any portion of the source text is
+/// selected. It can be used be used to obtain the raw source selection range.
+struct SourceSelectionRange {
nit: redundant `be used`.
Comment at: include/clang/Tooling/Refactoring/SourceSelectionConstraints.h:35
+/// A custom selection requirement.
+class Requirement {
+ /// Subclasses must implement 'T evaluateSelection(SelectionConstraint)
const'
It might worth explaining the relationship between this and the
`RequirementBase`.
Repository:
rL LLVM
https://reviews.llvm.org/D36075
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman added a comment. Ping. Repository: rL LLVM https://reviews.llvm.org/D36075 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman updated this revision to Diff 110563.
arphaman edited the summary of this revision.
arphaman added a comment.
- Simplify error/diagnostic handling. Use `DiagnosticOr` instead of
`DiagOr`.
- Simplify the code for the selection requirements by removing lambda deducers
and instead using special classes for requirements instead of lambdas/functions.
- Rename `selectionRequirement` to `requiredSelection`
Repository:
rL LLVM
https://reviews.llvm.org/D36075
Files:
include/clang/Basic/AllDiagnostics.h
include/clang/Basic/CMakeLists.txt
include/clang/Basic/Diagnostic.td
include/clang/Basic/DiagnosticIDs.h
include/clang/Basic/DiagnosticOr.h
include/clang/Basic/DiagnosticRefactoringKinds.td
include/clang/Basic/LLVM.h
include/clang/Tooling/Refactoring/AtomicChange.h
include/clang/Tooling/Refactoring/RefactoringActionRules.h
include/clang/Tooling/Refactoring/RefactoringDiagnostic.h
include/clang/Tooling/Refactoring/RefactoringOperationController.h
include/clang/Tooling/Refactoring/RefactoringResult.h
include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
lib/Basic/DiagnosticIDs.cpp
tools/diagtool/DiagnosticNames.cpp
unittests/Tooling/CMakeLists.txt
unittests/Tooling/RefactoringActionRulesTest.cpp
Index: unittests/Tooling/RefactoringActionRulesTest.cpp
===
--- /dev/null
+++ unittests/Tooling/RefactoringActionRulesTest.cpp
@@ -0,0 +1,157 @@
+//===- unittest/Tooling/RefactoringTestActionRulesTest.cpp ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ReplacementTest.h"
+#include "RewriterTestContext.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/RefactoringActionRules.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Support/Errc.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace tooling;
+using namespace refactoring_action_rules;
+
+namespace {
+
+class RefactoringActionRulesTest : public ::testing::Test {
+protected:
+ void SetUp() override {
+Context.Sources.setMainFileID(
+Context.createInMemoryFile("input.cpp", DefaultCode));
+ }
+
+ RewriterTestContext Context;
+ std::string DefaultCode = std::string(100, 'a');
+};
+
+TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
+ auto ReplaceAWithB =
+ [](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 Change;
+ };
+ class SelectionRequirement : public selection::Requirement {
+ public:
+std::pair
+evaluateSelection(selection::SourceSelectionRange Selection) const {
+ return std::make_pair(Selection, 20);
+}
+ };
+ auto Rule = apply(ReplaceAWithB, requiredSelection(SelectionRequirement()));
+
+ // When the requirements are satisifed, the rule's function must be invoked.
+ {
+RefactoringOperationController Operation(Context.Sources);
+SourceLocation Cursor =
+Context.Sources.getLocForStartOfFile(Context.Sources.getMainFileID())
+.getLocWithOffset(10);
+Operation.setSelectionRange({Cursor, Cursor});
+
+DiagnosticOr DiagOrResult = Rule->perform(Operation);
+ASSERT_FALSE(!DiagOrResult);
+RefactoringResult Result = std::move(*DiagOrResult);
+ASSERT_EQ(Result.getKind(), RefactoringResult::AtomicChanges);
+ASSERT_EQ(Result.getChanges().size(), 1u);
+std::string YAMLString = Result.getChanges()[0].toYAMLString();
+
+ASSERT_STREQ("---\n"
+ "Key: 'input.cpp:30'\n"
+ "FilePath:input.cpp\n"
+ "Error: ''\n"
+ "InsertedHeaders: \n"
+ "RemovedHeaders: \n"
+ "Replacements:\n" // Extra whitespace here!
+ " - FilePath:input.cpp\n"
+ "Offset: 30\n"
+ "Length: 1\n"
+ "ReplacementText: b\n"
+ "...\n",
+ YAMLString.c_str());
+ }
+
+ // When one of the requirements is not satisfied, perform should return either
+ // None or a valid diagnostic.
+ {
+RefactoringOperationController Operation(Context.Sources);
+DiagnosticOr DiagOrResult = Rule->perform(Operation);
+
+// A failure to select returns the invalidSelectionError.
+ASSERT_TRUE(!DiagOrResult);
+EXPECT_EQ(DiagOrResult.getDiagnostic().first,
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman added a comment. + Introduce refactoring diagnostics. Repository: rL LLVM https://reviews.llvm.org/D36075 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36075: [refactor] Initial support for refactoring action rules
arphaman created this revision.
Herald added a subscriber: mgorny.
This patch implements a couple of functions that were described in my RFC from
last week that’s available at
http://lists.llvm.org/pipermail/cfe-dev/2017-July/054831.html. This patch adds
the following pieces: `apply` function, `selectionRequirement` function, and
the `selection::SourceSelectionRange` constraint.
This code will be used as a base to start moving the functionality and tests
for clang-rename over to clang-refactor.
Unfortunately I had to use slightly different code for MSVC in the
`selectionRequirement` overloads that take a lambda as MSVC fails to deduce the
return type because of how it handles templates. I came up with a solution that
seems to work with MSVC 2015 (the min version supported by LLVM), and I’ve
tested the lambda overload code with cl 19.00.23506.
Repository:
rL LLVM
https://reviews.llvm.org/D36075
Files:
include/clang/Basic/LLVM.h
include/clang/Tooling/Refactoring/AtomicChange.h
include/clang/Tooling/Refactoring/DiagOr.h
include/clang/Tooling/Refactoring/RefactoringActionRules.h
include/clang/Tooling/Refactoring/RefactoringOperationController.h
include/clang/Tooling/Refactoring/RefactoringResult.h
include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
unittests/Tooling/CMakeLists.txt
unittests/Tooling/RefactoringActionRulesTest.cpp
Index: unittests/Tooling/RefactoringActionRulesTest.cpp
===
--- /dev/null
+++ unittests/Tooling/RefactoringActionRulesTest.cpp
@@ -0,0 +1,148 @@
+//===- unittest/Tooling/RefactoringTestActionRulesTest.cpp ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ReplacementTest.h"
+#include "RewriterTestContext.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/RefactoringActionRules.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace tooling;
+using namespace refactoring_action_rules;
+
+namespace {
+
+class RefactoringActionRulesTest : public ::testing::Test {
+protected:
+ void SetUp() override {
+Context.Sources.setMainFileID(
+Context.createInMemoryFile("input.cpp", DefaultCode));
+ }
+
+ RewriterTestContext Context;
+ std::string DefaultCode = std::string(100, 'a');
+};
+
+TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
+ auto ReplaceAWithB =
+ [](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 Change;
+ };
+ auto Rule =
+ apply(ReplaceAWithB,
+selectionRequirement([](selection::SourceSelectionRange Selection) {
+ return std::make_pair(Selection, 20);
+}));
+
+ // When the requirements are satisifed, the rule's function must be invoked.
+ {
+RefactoringOperationController Operation(Context.Sources);
+SourceLocation Cursor =
+Context.Sources.getLocForStartOfFile(Context.Sources.getMainFileID())
+.getLocWithOffset(10);
+Operation.setSelectionRange({Cursor, Cursor});
+
+DiagOr DiagOrResult = Rule->perform(Operation);
+ASSERT_FALSE(!DiagOrResult);
+Expected Result = std::move(*DiagOrResult);
+ASSERT_FALSE(!Result);
+ASSERT_EQ(Result->getKind(), RefactoringResult::AtomicChanges);
+ASSERT_EQ(Result->getChanges().size(), 1u);
+std::string YAMLString = Result->getChanges()[0].toYAMLString();
+
+ASSERT_STREQ("---\n"
+ "Key: 'input.cpp:30'\n"
+ "FilePath:input.cpp\n"
+ "Error: ''\n"
+ "InsertedHeaders: \n"
+ "RemovedHeaders: \n"
+ "Replacements:\n" // Extra whitespace here!
+ " - FilePath:input.cpp\n"
+ "Offset: 30\n"
+ "Length: 1\n"
+ "ReplacementText: b\n"
+ "...\n",
+ YAMLString.c_str());
+ }
+
+ // When one of the requirements is not satisfied, perform should return either
+ // None or a valid diagnostic.
+ {
+RefactoringOperationController Operation(Context.Sources);
+DiagOr DiagOrResult = Rule->perform(Operation);
+
+ASSERT_TRUE(!DiagOrResult);
+auto DiagOrNone = DiagOrResult.getDiag();
+ASSERT_TRUE(!DiagOrNone);
+ }
+}
+
+TEST_F(RefactoringActionRulesTest,
