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

[Kirill Bobyrev via cfe-commits]

omtcyfz added a comment.

Thank you for reviewing, @hokein!

Also, please note that this is not a final version, the interface will change a lot in the upcoming diffs.


================
Comment at: clang-refactor/driver/Driver.cpp:46
@@ +45,3 @@
+    llvm::StringRef FirstArgument(argv[1]);
+    if (FirstArgument == "-h" || FirstArgument == "-help" ||
+        FirstArgument == "--help")
----------------
hokein wrote:
> Any reason implementing the command-line options and not using the `cl::opt` here?
I might be mistaken, but it'd require parsing whole option sequence, which should be delegated to the submodules.

E.g. "help" should be only called if the the tool is invoked as `clang-refactor --help`, but if I parse all options I'd also invoke "help" while having "clang-refactor rename --help" for example. At least that's what I have been thinking.

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

Had an offline discussion with Alex about the interface and we came up to a conclusion that the interface should be way more strict. This one is just a general idea of how it might be implemented.

Thus, run shouldn't be overridden in my opinion, but I'll update the diff and make some high-level comments on that later.

================
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.
----------------
hokein wrote:
> Does the panic mean something like `exit(1)`? But from the interface, it looks like returning an error code, right?
1. Well, no. I believe that the error should be thrown to the `run` and "printed out" there.
2. Also, not that we're aiming for a multi-TU multi-threaded architecture in the end (although it doesn't make sense with the current interface, but again, I'll update it later).

================
Comment at: clang-refactor/modules/core/RefactoringModule.h:146
@@ +145,3 @@
+  // Panic: if there are conflicting replacements.
+  virtual int applyReplacements() = 0;
+
----------------
hokein wrote:
> 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 about multi-TU multi-threaded stuff */

Suppose a refactoring can't be applied to some translation units. Say, "rename" tool encountered name conflict in any TU. In this case the tool should fail and not process the last step.

If your comment is about naming only, I'll change it, too.

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

================
Comment at: clang-refactor/modules/template/TemplateModule.h:20
@@ +19,3 @@
+namespace template_module {
+
+class TemplateModule : public RefactoringModule {
----------------
hokein wrote:
> 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.
Deleting TemplateModule.


https://reviews.llvm.org/D24192