ioeric added inline comments.
================
Comment at: include/clang/Tooling/Refactoring/EditorCommandRegistry.def:5
+
+REFACTORING_EDITOR_COMMAND(ExtractFunction, "Extract Function")
+
----------------
Isn't `rename` also supported?
================
Comment at: include/clang/Tooling/Refactoring/EditorCommands.h:23
+/// Editor commands allow refactoring rules to be bound to commands that can
+/// be used from an editor that integrates with Clang's refactoring engine.
+///
----------------
Could you elaborate a bit on the editor/clangd integration? For example, if
clangd wants to extract a function, how would clangd discover the rule to use
and pass options into the engine? Would editor/clangd be responsible for
applying the changes?
================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:57
+
+ /// Returns the editor command that's was bound to this rule.
+ virtual const EditorCommand *getEditorCommand() { return nullptr; }
----------------
nit: s/that's was/that was/
================
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; }
};
----------------
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.
Repository:
rL LLVM
https://reviews.llvm.org/D38985
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
