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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits