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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:58
+  /// Returns the editor command that's was bound to this rule.
+  virtual const EditorCommand *getEditorCommand() { return nullptr; }
 };
----------------
arphaman wrote:
> ioeric wrote:
> > I'm still not quite sure about the intention of `EditorCommand` (is it supposed to be used as a mapping from name to rule, or the other way around?), but I'm a bit concerned about mixing editor logic with the refactoring rules this way. Also to enable a editor command, it seems to me that we would need to touch both the registry and the rule creation code, which seems a bit cumbersome.
> > 
> > I think we should either 1) separate out the command logic cleanly without touching action/rule interfaces in the refactoring engine or 2) simply bake the logic into the refactoring engine.
> > 
> > It is unclear to me if 1) is possible, but for 2), I think we could introduce a `RefactoringEngine` class which carries all refactoring actions as well as a map to serve the purpose of `EditorCommand`. And I think by doing this, we could also make the interfaces of `RefactoringEngine` more explicit.
> (Quick comment before the devmeeting:)
> Mapping from name to rule.
> You're right though, It might be better to explore an alternative solution, but I don't quite follow your proposal yet. I'll be in the tooling hacker's lab at the dev meeting today if you want to discuss this in person.
Currently, we have `createRefactoringActions` as the API of the refactoring engine/library.  I think we could introduce a `RefactoringEngine` class that exposes all user-facing interfaces from the engine, including creating/accessing actions and providing the mapping of editor command. And the command registration could be handled inside the class, which I assume would be simpler. This would also allow us to expose more interfaces in the future via this class.

(Sorry, I didn't make it to the dev meeting this year... I hope I could get a chance to meet you in the next dev meeting or the European dev meeting early next year :)


Repository:
  rL LLVM

https://reviews.llvm.org/D38985