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:
+ HeaderGenerator(llvm::StringRef HeaderName);
+
----------------
explicit.
================
Comment at: migrate-tool/MigrateTool.cpp:99
@@ +98,3 @@
+ MovedSymbols.push_back(Spec.getOldName());
+ // FIXME: consider source files. Need to determine whether source files send
+ // with ".cpp" or ".cc" etc.
----------------
s/send/end
================
Comment at: migrate-tool/MigrateTool.h:50
@@ +49,3 @@
+ public:
+ enum class MigrateType {
+ Class,
----------------
What is the 'MigrateType' used for? I can't find any usage in the patch.
================
Comment at: migrate-tool/RefactoringManager.h:28
@@ +27,3 @@
+ addIncludesToFiles(const std::set<llvm::StringRef> &Includes,
+ llvm::ArrayRef<std::string> Files) = 0;
+
----------------
The arguments `llvm::StringRef` and `std::string` look inconsistent to me at
first glance. But currently it is fine, we can modify it if needed in the
future.
https://reviews.llvm.org/D24380
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
