[PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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

[PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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

Re: [PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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

Re: [PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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

Re: [PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration 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

Re: [PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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?

Re: [PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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

Re: [PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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

Re: [PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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: +

Re: [PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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

Re: [PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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 >

Re: [PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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

Re: [PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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

Re: [PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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

Re: [PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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

Re: [PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

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: