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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

This is great work and definitely a lot to digest! ;) Some high-level comments for the first round.

In general, I really appreciate the high-level interfaces; I am just a bit concerned about the implementation which is a bit hard to follow at this point, especially with all the template magics. I left some suggestions in the comments; hopefully they would help.

I would also appreciate more detailed high-level comments on the major classes and their relationships - I found myself constantly going back to the RFC to understand their intentions.



================
Comment at: include/clang/Basic/DiagnosticOr.h:1
+//===--- DiagnosticOr.h - Diagnostic "closures" -----------------*- C++ -*-===//
+//
----------------
Could you please split changes related to `DiagnosticOr` into a separate patch and have owners of clang/Basic/ directory review it? ;)


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:1
+//===--- RefactoringActionRules.h - Clang refactoring library -------------===//
+//
----------------
Code in this file is a bit hard to follow, especially with so many classes and template magics :P 

Is it possible to split them into smaller modules, and ideally with more detailed documentation for each? 

It seems to me that the following logics could live in their own headers:
- `RefactoringActionRule` interface.
- Base classes for requirements.
- SourceSlection-related requirements.
- Derived/specialized rules.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:227
+std::unique_ptr<RefactoringActionRule>
+apply(Expected<RefactoringResult> (*RefactoringFunction)(
+          typename RequirementTypes::OutputType...),
----------------
Why is this called `apply`?  I feel something like `createAction` or `generateAction` would be more intuitive.


================
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
----------------
What are all the possible states, for example?


================
Comment at: include/clang/Tooling/Refactoring/RefactoringOperationController.h:22
+/// operations, by feeding the right information to the functions that
+/// evaluate the refactoring action rule requirements.
+class RefactoringOperationController {
----------------
Maybe I am missing too much context, but I found it hard to understand the comment and couldn't relate the comment to the public interfaces. Could you elaborate more? 

It is also unclear to me if this is specific to the source selection.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringOperationController.h:40
+private:
+  const SourceManager &SM;
+  SourceRange SelectionRange;
----------------
It's unclear to me what the intentions of these members are. Could you add some comments for these members? 


================
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:35
+
+  llvm::MutableArrayRef<AtomicChange> getChanges() {
+    assert(getKind() == AtomicChanges &&
----------------
Do we expect the result changes to be modified? Why?


================
Comment at: include/clang/Tooling/Refactoring/SourceSelectionConstraints.h:21
+/// This constraint is satisfied when any portion of the source text is
+/// selected. It can be used be used to obtain the raw source selection range.
+struct SourceSelectionRange {
----------------
nit: redundant `be used`.


================
Comment at: include/clang/Tooling/Refactoring/SourceSelectionConstraints.h:35
+/// A custom selection requirement.
+class Requirement {
+  /// Subclasses must implement 'T evaluateSelection(SelectionConstraint) const'
----------------
It might worth explaining the relationship between this and the `RequirementBase`. 


Repository:
  rL LLVM

https://reviews.llvm.org/D36075