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

[Kirill Bobyrev via cfe-commits]

omtcyfz added inline comments.

================
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;
----------------
curdeius wrote:
> 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);
>   }
> ```
The search is O(1) anyway, but the code itself probably becomes more reasonable, thanks.

================
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"
+
----------------
curdeius wrote:
> clang-format includes (and make it a single block)?
I like to separate includes into three groups (LLVM includes, STL includes, subproject includes), which I think is not very bad. Haven't seen any policy prohibiting it. Feel free to elaborate, though.

Sorted the includes within the second group.

================
Comment at: clang-refactor/modules/core/RefactoringModule.h:148
@@ +147,3 @@
+  // Panic: if there are conflicting replacements.
+  virtual int applyReplacementsOrOutputDiagnostics() = 0;
+
----------------
alexfh wrote:
> This should be possible to implement in a refactoring-independent way in clang-refactor itself. Or do you see any issues with this?
Nope, totally agree.


https://reviews.llvm.org/D24192