[PATCH] D36075: [refactor] Initial support for refactoring action rules

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.

Thanks for the changes! Lgtm with a few nits.



================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:33
+///
+///  - requiredSelection: The refactoring function won't be invoked unless the
+///                       given selection requirement is satisfied.
----------------
arphaman wrote:
> ioeric wrote:
> > We might want to document supported requirements somewhere else so that we don't need to update this file every time a new requirement is added.
> Do you think it should be in Clang's documentation? I can start on a new document there but I'd prefer to do it in a separate patch. WDYT?
Sure, this is fine for now. It would be nice to have proper documentation in the future when pieces get into places. 


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:33
+public:
+  virtual Expected<Optional<AtomicChanges>>
+  perform(RefactoringRuleContext &Context) = 0;
----------------
Why isn't this a interface in `SpecificRefactoringRuleAdapter` with return type `Expected<Optional<T>>`?


================
Comment at: unittests/Tooling/RefactoringActionRulesTest.cpp:67
+  {
+    RefactoringRuleContext Operation(Context.Sources);
+    SourceLocation Cursor =
----------------
It would be nice to also rename the variable from `Operation` to `Context`.


Repository:
  rL LLVM

https://reviews.llvm.org/D36075