[PATCH] D24192: [clang-refactor] introducing clang-refactor

[Marek Kurdej via cfe-commits]

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