ioeric added a comment.
Test phab...sorry spamming...
https://reviews.llvm.org/D24380
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ioeric updated this revision to Diff 73446.
ioeric added a comment.
Herald added a subscriber: modocache.
- Add SymbolRenameSpec; replace addIncludesToFiles and renameSymbolsInFiles
with renameSymbolsInAffectedFiles.
https://reviews.llvm.org/D24380
Files:
CMakeLists.txt
migrate-tool/Affect
ioeric added a comment.
Ping
https://reviews.llvm.org/D24380
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ioeric updated this revision to Diff 72635.
ioeric marked 2 inline comments as done.
ioeric added a comment.
- Replace dummy binary with unit test with dummy environemnt + fake codebase.
https://reviews.llvm.org/D24380
Files:
CMakeLists.txt
migrate-tool/AffectedFilesFinder.h
migrate-tool/
klimek added inline comments.
Comment at: migrate-tool/AffectedFilesFinder.h:24-25
@@ +23,4 @@
+public:
+ // Get all files that need to be updated when a symbol is renamed and/or
+ // moved.
+ virtual llvm::Expected>
Comment a bit on what the contract here is:
hokein added a comment.
The interfaces look good to me roughly.
Comment at: migrate-tool/MigrationEnvironment.h:22
@@ +21,3 @@
+// RefactoringManager, and AffectedFilesFinder.
+class MigrationEnvironment {
+public:
`MigrationContext` might be better?
==
ioeric added a comment.
In https://reviews.llvm.org/D24380#544720, @hokein wrote:
> Sorry for the delay.
> The patch only contains an unittest for `HeaderGenerato`r, which is not
> quite enough. Should we create a fake migrate-tool binary to illustrate APIs
> usage?
Done.
Good idea. I want
ioeric updated this revision to Diff 71845.
ioeric marked 4 inline comments as done.
ioeric added a comment.
- Added MigrationEnvironment class and a dummy migrate-tool binary.
https://reviews.llvm.org/D24380
Files:
CMakeLists.txt
migrate-tool/AffectedFilesFinder.h
migrate-tool/BuildManag
hokein added a comment.
Sorry for the delay.
The patch only contains an unittest for `HeaderGenerato`r, which is not quite
enough. Should we create a fake migrate-tool binary to illustrate APIs usage?
Comment at: migrate-tool/HeaderGenerator.h:22
@@ +21,3 @@
+public:
+ Header
ioeric added a comment.
Ping
https://reviews.llvm.org/D24380
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ioeric added inline comments.
Comment at: migrate-tool/HeaderBuild.h:29
@@ +28,3 @@
+
+ std::string generateContent() const;
+
klimek wrote:
> This all needs more comments :)
> I assume we'll also need to somehow give this a "style" at some point? Or
> alternati
ioeric updated this revision to Diff 71116.
ioeric marked 13 inline comments as done.
ioeric added a comment.
Herald added a subscriber: mgorny.
- Addressed review comments.
- Separate getAffectedFiles into its own interface.
https://reviews.llvm.org/D24380
Files:
CMakeLists.txt
migrate-too
klimek added inline comments.
Comment at: migrate-tool/BuildManager.h:22
@@ +21,3 @@
+public:
+ virtual bool addHeaderOnlyLibrary(llvm::StringRef HeaderPath) = 0;
+
I'm not sure the header-only distinction makes sense.
I'd start with
virtual bool addLibrary(llvm:
bkramer added a subscriber: bkramer.
bkramer added a comment.
One round of llvm-API specifics.
Comment at: migrate-tool/BuildManager.h:22
@@ +21,3 @@
+public:
+ virtual bool addHeaderOnlyLibrary(llvm::StringRef HeaderPath) = 0;
+
These methods could use some do
omtcyfz added a comment.
In https://reviews.llvm.org/D24380#538556, @ioeric wrote:
> In https://reviews.llvm.org/D24380#538434, @Eugene.Zelenko wrote:
>
> > I think will be good idea to await clang-refactor and merge code there.
>
>
> This tool is not exactly a clang-tool; it is a framework that
ioeric added a comment.
In https://reviews.llvm.org/D24380#538434, @Eugene.Zelenko wrote:
> I think will be good idea to await clang-refactor and merge code there.
This tool is not exactly a clang-tool; it is a framework that invokes multiple
clang refactoring tool and also manipulates build r
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.
I think will be good idea to await clang-refactor and merge code there.
Please run Include What You Use. Code use a lot of containers and will be good
to include them explicitly.
Comment at: mig
17 matches
Mail list logo
