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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringOperationController.h:19
+
+/// Encapsulates all of the possible state that an individual refactoring
+/// operation might have. Controls the process of initiation of refactoring
----------------
ioeric wrote:
> What are all the possible states, for example?
Bad comment. I think `inputs` is more descriptive.


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


================
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:35
+
+  llvm::MutableArrayRef<AtomicChange> getChanges() {
+    assert(getKind() == AtomicChanges &&
----------------
ioeric wrote:
> Do we expect the result changes to be modified? Why?
No, that was a workaround for a non-const member use. I'll use a const cast instead.


================
Comment at: include/clang/Tooling/Refactoring/SourceSelectionConstraints.h:35
+/// A custom selection requirement.
+class Requirement {
+  /// Subclasses must implement 'T evaluateSelection(SelectionConstraint) const'
----------------
ioeric wrote:
> It might worth explaining the relationship between this and the `RequirementBase`. 
This class is not related to `RequirementBase` though. Were you talking about another class?


Repository:
  rL LLVM

https://reviews.llvm.org/D36075