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:41
@@ +40,3 @@
+ Command = argv[1];
+ std::string Invocation = std::string(argv[0]) + " " + argv[1];
+ argv[1] = Invocation.c_str();
----------------
Simpler:
```
std::string Invocation = argv[0] + (" " + Command);
```
================
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[CommandToModuleID[Command]]->run(argc, argv);
+ }
+ return 1;
----------------
Using `find` and then `operator[]` makes the search twice. IMO, it would be
better to avoid that by:
```
auto it = CommandToModuleID.find(Command);
if (it != CommandToModuleID.end()) {
return RegisteredModules[*it]->run(argc, argv);
}
```
================
Comment at: clang-refactor/driver/ModuleManager.h:13-19
@@ +12,9 @@
+
+#include "llvm/ADT/StringRef.h"
+
+#include <string>
+#include <vector>
+#include <unordered_map>
+
+#include "core/RefactoringModule.h"
+
----------------
clang-format includes (and make it a single block)?
================
Comment at: clang-refactor/driver/ModuleManager.h:21
@@ +20,3 @@
+
+using namespace llvm;
+
----------------
`using namespace` in header?!
================
Comment at: clang-refactor/modules/core/RefactoringModule.h:36
@@ +35,3 @@
+ //
+ // 1. Extract infromation needed for the refactoring upon tool invocation.
+ //
----------------
s/infromation/information
================
Comment at: clang-refactor/modules/core/RefactoringModule.h:56
@@ +55,3 @@
+ // 2. Check whether renaming will introduce any name conflicts. If it won't
+ // find each occurance of the symbol in translation unit using USR and store
+ // replacements.
----------------
s/occurance/occurrence
================
Comment at: clang-refactor/modules/core/RefactoringModule.h:72
@@ +71,3 @@
+ // code will no longer compile. If it won't find function calls, add needed
+ // temprorary variables and replace the call with function body.
+ //
----------------
s/temprorary/temporary
================
Comment at: clang-refactor/modules/core/RefactoringModule.h:112
@@ +111,3 @@
+
+ // Routine for regiestering common modules options.
+ void registerCommonOptions(llvm::cl::OptionCategory &Category) {
----------------
s/regiestering/registering
https://reviews.llvm.org/D24192
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
