[PATCH] D38985: [refactor] Add support for editor commands that connect IDEs/editors to the refactoring actions

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, I think this is looking much better.
A couple of bits of the interface could be yet simpler :-)



================
Comment at: include/clang/Tooling/Refactoring/Extract/ExtractFunction.h:21
+/// then called from the place where the original code was.
+class ExtractFunction final : public SourceChangeRefactoringRule {
+public:
----------------
The interface given here looks great, much simpler to use directly!

The inheritance gives me some pause. A couple of related things are going on here:
 - we're inheriting the `RefactoringActionRuleBase` interface, which allows this refactoring to be used generically
 - we're inheriting `SourceChangeRefactoringRule`, which adapts the generic interface to the one we actually implement

As consequences:
  - we're bound to use a fairly generic name as our entry point here (but I actually think it's a good name, so no worries)
  - we advertise *two* public entry point, that do the same thing (but it's not obvious from the signature)

The thing is, I don't think people who want the "generic" interface will be using this class directly - they'll be going through the Engine, right?
So `ExtractFunction` -> `RefactoringActionRuleBase` adapter doesn't need to be here, I think it can be private in the Engine.
That seems like a clearer separation of concerns: `ExtractFunction` only cares about getting its work done, and `RefactoringEngine` owns exposing a generic interface to all the rules.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:26
+/// Describes a refactoring rule.
+class RefactoringDescriptorInterface {
+public:
----------------
Does this need to be an interface + implementation + template parameter - can it just be a struct? I think it's static data per instance.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:30
+
+  /// Returns the name of the refactoring.
+  virtual StringRef getName() const = 0;
----------------
nit: the comment could be more specific: "unique identifier" would make it clearer this isn't primarily human-readable.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:38
+  ///
+  /// When the refactoring has no title, it won't be available in an editor
+  /// unless the editor client explicitly supports the refactoring through
----------------
I think this is conflating two concepts:
 - what's the best short human-readable name for the refactoring
 - do we want the refactoring turned on by "automatically" for editors

I'm not sure this library is the right place to decide what set of features an editor should present - at least for clangd I'd prefer to have the editor own a whitelist.
Either way, it'd be great to separate out the two fields here.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:79
+  /// Returns the descriptor for this refactoring rule.
+  virtual const RefactoringDescriptorInterface &getDescriptor() = 0;
 };
----------------
Did you intend to put this in RefactoringActionRuleBase? It'd be nice to have it for all types of rules.

Also, it's an instance method, but don't you want to be able to get at the information even if you can't apply the refactoring in the given context (so initiate fails).

Maybe this should be static (and `RefactoringEngine` should expose it as part of its generic interface)


================
Comment at: lib/Tooling/Refactoring/Extract.cpp:19
 #include "clang/Rewrite/Core/Rewriter.h"
-#include "clang/Tooling/Refactoring/RefactoringAction.h"
-#include "clang/Tooling/Refactoring/RefactoringActionRules.h"
-#include "clang/Tooling/Refactoring/RefactoringOptions.h"
+#include "clang/Tooling/Refactoring/Extract/ExtractFunction.h"
 
----------------
nit: The cpp file is "Extract" but the header is "Extract/ExtractFunction.h" - are we expecting lots more extractions?


================
Comment at: lib/Tooling/Refactoring/RefactoringActions.cpp:24
+
+template <>
+class RefactoringDescriptor<ExtractFunction>
----------------
It'd be nice if the actual names remained near the rules themselves I think.
Could be quite simple, like

   ExtractFunction {
    ...
    static const RefactoringDescriptor& describe() {
      static RefactoringDescriptor D{
        "extract-function",
        "Extract Function",
        "Extracts code into a new function",
       };
      return D;
    }
  }


Repository:
  rL LLVM

https://reviews.llvm.org/D38985