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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
