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