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

[Haojian Wu via cfe-commits]

hokein added a comment.

> Eric's point was that this patch should only care about clang-refactor and introduce changes directly related to creating clang-rename. clang-rename and all other tools migration can be done later, which is also good in terms of this patch not growing too large.


I'm +1 on making this patch implement a skeleton of clang-refactor, which makes it small, and much easier for review. The patch looks much better now.


================
Comment at: clang-refactor/driver/Driver.cpp:10
@@ +9,3 @@
+///
+///  \file This file implements a clang-tidy tool.
+///
----------------
clang-tidy?

================
Comment at: clang-refactor/driver/Driver.cpp:37
@@ +36,3 @@
+
+int clanRefactorMain(int argc, const char **argv) {
+  ModuleManager Manager;
----------------
s/clan/clang

================
Comment at: clang-refactor/driver/Driver.cpp:46
@@ +45,3 @@
+    llvm::StringRef FirstArgument(argv[1]);
+    if (FirstArgument == "-h" || FirstArgument == "-help" ||
+        FirstArgument == "--help")
----------------
Any reason implementing the command-line options and not using the `cl::opt` here?

================
Comment at: clang-refactor/driver/ModuleManager.h:24
@@ +23,3 @@
+public:
+  void AddModule(std::string Command, std::unique_ptr<RefactoringModule> Module);
+
----------------
`StringRef`.

================
Comment at: clang-refactor/modules/core/RefactoringModule.h:43
@@ +42,3 @@
+  //
+  // Few examples of how each of these stages wouold look like for future
+  // modules "rename" and "inline".
----------------
s/wouold/would/

================
Comment at: clang-refactor/modules/core/RefactoringModule.h:54
@@ +53,3 @@
+  //
+  // 2. Check whether renaming will introduce any name conflicts. It it won't
+  // find each occurance of the symbol in translation unit using USR and store
----------------
"It it" doesn't make sense here.

================
Comment at: clang-refactor/modules/core/RefactoringModule.h:89
@@ +88,3 @@
+  //
+  int run(int argc, const char **argv) {
+    // Register options.
----------------
I'd make this interface a virtual function too, and provide this default implementation.

================
Comment at: clang-refactor/modules/core/RefactoringModule.h:128
@@ +127,3 @@
+  //
+  // Panic: if refactoring can not be applied. E.g. unsupported cases like
+  // renaming macros etc.
----------------
Does the panic mean something like `exit(1)`? But from the interface, it looks like returning an error code, right?

================
Comment at: clang-refactor/modules/core/RefactoringModule.h:146
@@ +145,3 @@
+  // Panic: if there are conflicting replacements.
+  virtual int applyReplacements() = 0;
+
----------------
A further thought:

`applyReplacement` is more likely a postprocess step of `handleTranslationUnit`, what if some clang-refactor subtools just want to dump the results only? I'd prefer to rename it to `OnEndOfHandlingTranslationUnit`. I'm open to hear better suggestions. 

================
Comment at: clang-refactor/modules/core/RefactoringModule.h:148
@@ +147,3 @@
+
+  virtual ~RefactoringModule() {}
+
----------------
Use "= default".

================
Comment at: clang-refactor/modules/core/RefactoringModule.h:150
@@ +149,3 @@
+
+  const std::string &getModuleName() { return ModuleName; }
+
----------------
You can return a `StringRef` here?

================
Comment at: clang-refactor/modules/core/RefactoringModule.h:152
@@ +151,3 @@
+
+  const std::string &getModuleMetaDescription() { return ModuleMetaDescription; }
+
----------------
The same.

================
Comment at: clang-refactor/modules/template/TemplateModule.h:15
@@ +14,3 @@
+
+#include <string>
+
----------------
Not needed.

================
Comment at: clang-refactor/modules/template/TemplateModule.h:20
@@ +19,3 @@
+namespace template_module {
+
+class TemplateModule : public RefactoringModule {
----------------
It'd be clear to add a small paragraph describing that this is a template module which is only used to lower the "barrier to entry" for writing clang-refactor subtool.


https://reviews.llvm.org/D24192