ioeric added inline comments.
Comment at: clang-refactor/driver/Driver.cpp:36
@@ +35,3 @@
+argv[1] = (llvm::Twine(argv[0]) + " " +
llvm::Twine(argv[1])).str().c_str();
+Manager.dispatch(Command, argc - 1, argv + 1);
+ } else {
arphaman wrote:
> Is there
arphaman added inline comments.
Comment at: clang-refactor/driver/Driver.cpp:36
@@ +35,3 @@
+argv[1] = (llvm::Twine(argv[0]) + " " +
llvm::Twine(argv[1])).str().c_str();
+Manager.dispatch(Command, argc - 1, argv + 1);
+ } else {
Is there a particular rea
arphaman added inline comments.
Comment at: clang-refactor/core/SymbolIndexClient.h:31
@@ +30,3 @@
+} // end namespace refactor
+} // end namespace refactor
+
End namespace clang?
https://reviews.llvm.org/D24192
___
ioeric added inline comments.
Comment at: clang-refactor/driver/ModuleManager.h:14-20
@@ +13,9 @@
+#include "clang/Basic/LLVM.h"
+#include "llvm/ADT/StringRef.h"
+
+#include
+#include
+#include
+
+#include "core/RefactoringModule.h"
+
curdeius wrote:
> I though
omtcyfz updated this revision to Diff 71776.
omtcyfz added a comment.
Herald added a subscriber: mgorny.
The diff doesn't really do much right now, but it kind of gives the idea of
what's happening with clang-refactor.
https://reviews.llvm.org/D24192
Files:
CMakeLists.txt
clang-refactor/CM
omtcyfz updated this revision to Diff 70802.
omtcyfz marked 2 inline comments as done.
omtcyfz added a comment.
Slightly improve the interface.
Patch is still not complete, though.
https://reviews.llvm.org/D24192
Files:
CMakeLists.txt
clang-refactor/CMakeLists.txt
clang-refactor/driver/C
curdeius added inline comments.
Comment at: clang-refactor/driver/ModuleManager.h:14-20
@@ +13,9 @@
+#include "clang/Basic/LLVM.h"
+#include "llvm/ADT/StringRef.h"
+
+#include
+#include
+#include
+
+#include "core/RefactoringModule.h"
+
I thought that idea behi
omtcyfz added inline comments.
Comment at: clang-refactor/driver/ModuleManager.cpp:22-24
@@ +21,5 @@
+int ModuleManager::Dispatch(StringRef Command, int argc, const char **argv) {
+ if (CommandToModuleID.find(Command) != CommandToModuleID.end()) {
+return RegisteredModules[Co
omtcyfz updated this revision to Diff 70682.
omtcyfz marked 8 inline comments as done.
omtcyfz added a comment.
Herald added a subscriber: beanz.
Addressing few comments.
Major improvements on the way.
https://reviews.llvm.org/D24192
Files:
CMakeLists.txt
clang-refactor/CMakeLists.txt
cl
alexfh requested changes to this revision.
This revision now requires changes to proceed.
Comment at: clang-refactor/driver/Driver.cpp:41
@@ +40,3 @@
+Command = argv[1];
+std::string Invocation = std::string(argv[0]) + " " + argv[1];
+argv[1] = Invocation.c_str();
alexfh added a comment.
Kirill, first, thank you for working on this!
As discussed offline, the RefactoringModule interface should explicitly define:
- separation of execution stages for each check:
1. preparation or execution planning - should figure out the set of affected
translation units
curdeius added a subscriber: curdeius.
curdeius added a comment.
For the moment, just a few nitty-gritty comments inline.
What I miss here is (as already pointed by someone) an example on how to write
a new module, register it etc.
Comment at: clang-refactor/driver/Driver.cpp:4
omtcyfz added a comment.
Thank you for reviewing, @hokein!
Also, please note that this is not a final version, the interface will change a
lot in the upcoming diffs.
Comment at: clang-refactor/driver/Driver.cpp:46
@@ +45,3 @@
+llvm::StringRef FirstArgument(argv[1]);
+i
omtcyfz updated this revision to Diff 70397.
omtcyfz marked 11 inline comments as done.
omtcyfz added a comment.
Addressing a round of comments.
https://reviews.llvm.org/D24192
Files:
CMakeLists.txt
clang-refactor/CMakeLists.txt
clang-refactor/driver/CMakeLists.txt
clang-refactor/driver
hokein added a comment.
> Eric's point was that this patch should only care about clang-refactor and
> introduce changes directly related to creating clang-rename. clang-rename and
> all other tools migration can be done later, which is also good in terms of
> this patch not growing too large.
omtcyfz added a comment.
Bringing results of an offline discussion with Eric (@ioeric) live.
Eric's point was that this patch should only care about `clang-refactor` and
introduce changes directly related to creating `clang-rename`. `clang-rename`
and all other tools migration can be done later
omtcyfz updated the summary for this revision.
omtcyfz removed a reviewer: bkramer.
omtcyfz added subscribers: bkramer, hokein.
omtcyfz updated this revision to Diff 70373.
omtcyfz added a comment.
Removed whole clang-rename part, only making this patch clang-rename specific.
https://reviews.llv
omtcyfz added a comment.
In https://reviews.llvm.org/D24192#533220, @ioeric wrote:
> Don't worry about a patch being small. Reviewers like small patches :)
>
> The reason that I suggested a dummy sub-tool is to lower the bar for
> developers, especially those who have never developed a clang too
omtcyfz added a comment.
In https://reviews.llvm.org/D24192#533233, @ioeric wrote:
> It was not trivial to me why USREngine is so important to those tools. You
> might want to address that in the design doc as well. And given the weight
> USREngine carries in clang-refactor as you suggested, I
alexshap added a subscriber: alexshap.
Comment at: clang-refactor/driver/Rename.h:192
@@ +191,3 @@
+ auto ID = Sources.translateFile(Entry);
+ Rewrite.getEditBuffer(ID).write(outs());
+}
if i am not mistaken if the file has not been modified this wil
ioeric added a comment.
In https://reviews.llvm.org/D24192#533200, @omtcyfz wrote:
> In https://reviews.llvm.org/D24192#533198, @ioeric wrote:
>
> > In https://reviews.llvm.org/D24192#533174, @omtcyfz wrote:
> >
> > > In https://reviews.llvm.org/D24192#532981, @ioeric wrote:
> > >
> > > > - It wo
ioeric added a comment.
In https://reviews.llvm.org/D24192#533082, @omtcyfz wrote:
> In https://reviews.llvm.org/D24192#532981, @ioeric wrote:
>
> > - You mentioned a design doc in the summary; maybe also include a link to
> > it?
>
>
> Done.
>
> > - It would make the review easier if you could
omtcyfz added a comment.
In https://reviews.llvm.org/D24192#533198, @ioeric wrote:
> In https://reviews.llvm.org/D24192#533174, @omtcyfz wrote:
>
> > In https://reviews.llvm.org/D24192#532981, @ioeric wrote:
> >
> > > - It would make the review easier if you could separate the migration of
> > >
ioeric added a comment.
In https://reviews.llvm.org/D24192#533174, @omtcyfz wrote:
> In https://reviews.llvm.org/D24192#532981, @ioeric wrote:
>
> > - It would make the review easier if you could separate the migration of
> > clang-rename into another patch...
>
>
> Another point is that if I tr
omtcyfz added a comment.
In https://reviews.llvm.org/D24192#532981, @ioeric wrote:
> - It would make the review easier if you could separate the migration of
> clang-rename into another patch...
Another point is that if I try to separate the migration - what do I do about
USREngine? USREngine
omtcyfz updated this revision to Diff 70194.
omtcyfz added a comment.
Revert diff, as the last one "deletes and creates" files instead of "moving and
changing them" in the filesystem.
https://reviews.llvm.org/D24192
Files:
CMakeLists.txt
TemplatedClassFunction.cpp
clang-refactor/CMakeLis
omtcyfz added a comment.
In https://reviews.llvm.org/D24192#532981, @ioeric wrote:
> - You mentioned a design doc in the summary; maybe also include a link to it?
Done.
> - It would make the review easier if you could separate the migration of
> clang-rename into another patch...I think clang
ioeric added a comment.
- You mentioned a design doc in the summary; maybe also include a link to it?
- It would make the review easier if you could separate the migration of
clang-rename into another patch...I think clang-refactor is really the
interesting part here. And maybe also add a small
28 matches
Mail list logo
