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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for the changes! The code is much clearer.

I am wondering if the current design could be extended to support tools (or rules) that use AST matchers? Or is the selection expected to be powerful enough to replace AST matchers?

We have a few tools in `clang-tools-extra` (e.g. `clang-move` and `change-namespace`) and many internal tools that are based on AST matchers, but we would really like to move them into `clang-refactor` in the future :)



================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:26
+template <typename T>
+detail::SourceSelectionRequirement<
+    typename selection::detail::EvaluateSelectionChecker<
----------------
Could you help me understand this class? 

This seems to be a selection-specific requirement and should live in `selection`. It is also used in `BaseSpecializedRule` which seems to be a higher level of abstraction. 


================
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:27
+
+  RefactoringResult(AtomicChange Change) : Kind(AtomicChanges) {
+    Changes.push_back(std::move(Change));
----------------
arphaman wrote:
> alexshap wrote:
> > explicit ?
> Nah, it's more convenient to be able to return a single `AtomicChanges` without an explicit initializer I think.
+1 to `explicit` which could prevent unintentional conversion. `RefactoringResult(Change);` isn't too bad IMO.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:21
+struct RefactoringResult {
+  enum ResultKind {
+    /// A set of source replacements represented using a vector of
----------------
I'm a bit unsure about the abstraction of the refactoring result. I would expected refactoring results to be source changes always. Do you have any refactoring tool that outputs occurrences in mind?


================
Comment at: include/clang/Tooling/Refactoring/SourceSelectionConstraints.h:55
+
+namespace detail {
+
----------------
Does `detail` mean internal implementation? Maybe use `internal` which is more commonly used for this?


Repository:
  rL LLVM

https://reviews.llvm.org/D36075