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