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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman marked 5 inline comments as done.
arphaman added inline comments.


================
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:
----------------
sammccall wrote:
> 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.
You're right about the dual adaption, but this kind of class **should not** be invoked directly for the following reason:
While it's easy to call `createSourceReplacements` or `invoke` for local refactorings, the cross-TU actions will need a different invocation harness around them. Clangd should avoid using this kind of class directly and use another harness that can run refactorings across multiple-TUs if needed.

I think it's best if it stays right now, as it will help to drive cross-TU refactorings. We can reconsider this in the future though.
I made the `createSourceReplacements` here private though.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:79
+  /// Returns the descriptor for this refactoring rule.
+  virtual const RefactoringDescriptorInterface &getDescriptor() = 0;
 };
----------------
sammccall wrote:
> 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)
Classes like ExtractFunction could be reused by multiple rules, so I put it there before to keep the "is this refactoring available in editor" associated with a rule. But now with a more generic descriptor that doesn't really associate title with "is this refactoring available in an editor" I can put the descriptor into the ExtractFunction class itself.


Repository:
  rL LLVM

https://reviews.llvm.org/D38985