arphaman updated this revision to Diff 113533.
arphaman added a comment.
Rebase on top of trunk and https://reviews.llvm.org/D37210.
Ping.
Repository:
rL LLVM
https://reviews.llvm.org/D36574
Files:
include/clang/Tooling/Refactoring/RefactoringAction.h
include/clang/Tooling/Refactoring/R
klimek added a comment.
One of my main concerns is still that I don't see the need for all the template
magic yet :) Why doesn't everybody use the RefactoringResult we define here?
Comment at: test/Refactor/LocalRename/Field.cpp:4
+class Baz {
+ int /*range=*/Foo; // CHECK: s
arphaman added a comment.
In https://reviews.llvm.org/D36574#858763, @klimek wrote:
> One of my main concerns is still that I don't see the need for all the
> template magic yet :) Why doesn't everybody use the RefactoringResult we
> define here?
This refactoring result is only really useful
arphaman added inline comments.
Comment at: test/Refactor/LocalRename/Field.cpp:4
+class Baz {
+ int /*range=*/Foo; // CHECK: symbol [[@LINE]]:17 -> [[@LINE]]:20
+public:
klimek wrote:
> Does this just test the selection?
No, this is the moved `clang-rename/Fiel
arphaman updated this revision to Diff 113736.
arphaman marked an inline comment as done.
arphaman added a comment.
Get rid of inheritance of `RefactoringEngine` by getting rid of the class
itself.
Repository:
rL LLVM
https://reviews.llvm.org/D36574
Files:
include/clang/Tooling/Refactorin
arphaman added a comment.
In https://reviews.llvm.org/D36574#858763, @klimek wrote:
> One of my main concerns is still that I don't see the need for all the
> template magic yet :) Why doesn't everybody use the RefactoringResult we
> define here?
@klimek, are you talking about the template us
klimek added a comment.
In https://reviews.llvm.org/D36574#860197, @arphaman wrote:
> In https://reviews.llvm.org/D36574#858763, @klimek wrote:
>
> > One of my main concerns is still that I don't see the need for all the
> > template magic yet :) Why doesn't everybody use the RefactoringResult w
ioeric added inline comments.
Comment at: include/clang/Tooling/Refactoring/RefactoringAction.h:20
+
+class RefactoringAction {
+public:
Please add documentation for the class.
I appreciate your RFC that explains all these concepts; it would be nice if you
cou
arphaman added a comment.
In https://reviews.llvm.org/D36574#860203, @klimek wrote:
> In https://reviews.llvm.org/D36574#860197, @arphaman wrote:
>
> > In https://reviews.llvm.org/D36574#858763, @klimek wrote:
> >
> > > One of my main concerns is still that I don't see the need for all the
> > >
klimek added inline comments.
Comment at: test/Refactor/LocalRename/Field.cpp:4
+class Baz {
+ int /*range=*/Foo; // CHECK: symbol [[@LINE]]:17 -> [[@LINE]]:20
+public:
arphaman wrote:
> klimek wrote:
> > arphaman wrote:
> > > klimek wrote:
> > > > Does this jus
arphaman updated this revision to Diff 113804.
arphaman marked 8 inline comments as done.
arphaman added a comment.
- Test by checking the modified source directly. This allowed me to get rid of
the refactoring result wrapper as well.
- Remove ASTRefactoringContext
- Address the other review comm
arphaman added inline comments.
Comment at: tools/clang-refactor/ClangRefactor.cpp:60
+ : SubCommand(Name, Description), Action(&Action) {
+Sources = llvm::make_unique>(
+cl::Positional, cl::ZeroOrMore, cl::desc(" [... ]"),
arphaman wrote:
> ioer
ioeric added inline comments.
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:45
std::unique_ptr
createRefactoringRule(Expected (*RefactoringFunction)(
typename RequirementTypes::OutputType...),
Can we get rid o
arphaman added inline comments.
Comment at:
include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:99
+ template
+ void invokeImplDispatch(
+ RefactoringResultConsumer &Consumer, RefactoringRuleContext &,
ioeric wrote:
> Do we still need this
arphaman updated this revision to Diff 113826.
arphaman marked 6 inline comments as done.
arphaman added a comment.
- Always pass in `RefactoringRuleContext` to the refactoring rule's function.
- Remove now redundant overloads.
- Make LocalRename's actions members private.
- Add CommandLienParser
ioeric added inline comments.
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:43
+class LocalRename : public RefactoringAction {
+ StringRef getCommand() const override { return "local-rename"; }
+
Shouldn't these be public?
Comm
arphaman added inline comments.
Comment at: tools/clang-refactor/ClangRefactor.cpp:103
+IsSelectionParsed = true;
+// FIXME: Support true selection ranges.
+StringRef Value = *Selection;
ioeric wrote:
> Is the test selection temporary before we have t
arphaman updated this revision to Diff 113981.
arphaman marked 3 inline comments as done.
arphaman added a comment.
Address review comments
Repository:
rL LLVM
https://reviews.llvm.org/D36574
Files:
include/clang/Tooling/Refactoring/AtomicChange.h
include/clang/Tooling/Refactoring/Refact
ioeric added inline comments.
Comment at: tools/clang-refactor/ClangRefactor.cpp:103
+IsSelectionParsed = true;
+// FIXME: Support true selection ranges.
+StringRef Value = *Selection;
arphaman wrote:
> ioeric wrote:
> > Is the test selection temporar
arphaman marked an inline comment as done.
arphaman added inline comments.
Comment at: tools/clang-refactor/ClangRefactor.cpp:79
+
+ RefactoringAction &getAction() const { return *Action; }
+
ioeric wrote:
> Why return a mutable reference? Also, this is not used
arphaman updated this revision to Diff 114125.
arphaman added a comment.
Refactor the selection argument handling by using a `SourceSelectionArgument`
interface with subclass for test selection and separate out the test code from
the tool class.
Repository:
rL LLVM
https://reviews.llvm.org/
ioeric added inline comments.
Comment at: tools/clang-refactor/ClangRefactor.cpp:29
+using namespace tooling;
+using namespace clang_refactor;
+namespace cl = llvm::cl;
Sorry for haven't noticed this earlier, but I think `clang::refactor` sounds
like a better na
arphaman updated this revision to Diff 114309.
arphaman marked 2 inline comments as done.
arphaman added a comment.
- rename namespace
- try to further decouple SourceSelectionArgument
Repository:
rL LLVM
https://reviews.llvm.org/D36574
Files:
include/clang/Tooling/Refactoring/AtomicChange
arphaman added inline comments.
Comment at: tools/clang-refactor/ClangRefactor.cpp:62
+ /// \returns true if an error occurred, false otherwise.
+ virtual bool refactorForEachSelection(
+ RefactoringRuleContext &Context,
ioeric wrote:
> I would expect the
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
I think this is ready to go.
@klimek Manuel, are all your concerns addressed?
Comment at: tools/clang-refactor/ClangRefactor.cpp:62
+ /// \returns true if an error occurred
ioeric added a comment.
Please wait for Manuel's reply before landing the patch ;-)
Repository:
rL LLVM
https://reviews.llvm.org/D36574
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com
arphaman added a comment.
@klimek I've added a patch that simplifies the interface and the template code
- https://reviews.llvm.org/D37681. Hopefully you'll find that the code there is
cleaner.
Repository:
rL LLVM
https://reviews.llvm.org/D36574
__
ioeric added a comment.
Feel free to land the patch now if comments are addressed.
Repository:
rL LLVM
https://reviews.llvm.org/D36574
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-comm
klimek accepted this revision.
klimek added a comment.
LG. Nice, thanks!
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:42
+
+class LocalRename : public RefactoringAction {
+public:
I have to admit, the implementation here is pretty neat! :)
Re
hokein accepted this revision.
hokein added a comment.
LGTM, a few nits.
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:42
+
+class LocalRename : public RefactoringAction {
+public:
klimek wrote:
> I have to admit, the implementation here is pret
This revision was automatically updated to reflect the committed changes.
Closed by commit rL313025: [refactor] add a refactoring action rule that
returns symbol occurrences (authored by arphaman).
Changed prior to commit:
https://reviews.llvm.org/D36574?vs=114309&id=114806#toc
Repository:
r
arphaman reopened this revision.
arphaman added a comment.
This revision is now accepted and ready to land.
Oops, I've put in the wrong diff link for r313025. This patch is still not
committed.
Repository:
rL LLVM
https://reviews.llvm.org/D36574
___
arphaman marked 8 inline comments as done.
arphaman added inline comments.
Comment at: tools/clang-refactor/TestSupport.h:32
+
+namespace refactor {
+
hokein wrote:
> Do you plan to use `refactor` on other files?
Probably in the future, yeah.
Repository:
rL
hokein added a comment.
Seems that you have attached a wrong diff, which is committed in r313025.
Comment at: tools/clang-refactor/TestSupport.h:32
+
+namespace refactor {
+
arphaman wrote:
> hokein wrote:
> > Do you plan to use `refactor` on other files?
> Pr
arphaman added a comment.
Yeah, I accidentally committed a prior patch with the link to this diff. I will
commit this patch today though so it should be updated.
Repository:
rL LLVM
https://reviews.llvm.org/D36574
___
cfe-commits mailing list
cf
This revision was automatically updated to reflect the committed changes.
Closed by commit rL313244: [refactor] add clang-refactor tool with initial
testing support and (authored by arphaman).
Changed prior to commit:
https://reviews.llvm.org/D36574?vs=114806&id=115191#toc
Repository:
rL LLV
arphaman created this revision.
Herald added a subscriber: mgorny.
This patch depends on https://reviews.llvm.org/D36075 and
https://reviews.llvm.org/D36156.
It introduces the `clang-refactor` tool alongside the `local-rename` action
which is uses the existing renaming engine used by `clang-ren
37 matches
Mail list logo
