[PATCH] D35012: [refactor] Add the AST source selection component

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Some nits.



================
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:23
+/// of the cursor or overlap with the selection range.
+class ASTSelectionFinder : public RecursiveASTVisitor<ASTSelectionFinder> {
+  const SourceLocation Location;
----------------
In LLVM, we use the following class layout:

```
class X {
public:
  void f();

private:
  void g();

  int member;
}
```


================
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:76
+
+  Optional<SelectedASTNode> getResult() {
+    assert(SelectionStack.size() == 1 && "stack was not popped");
----------------
I think `getSelectedASTNode` would be clearer.


================
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:90
+      return RecursiveASTVisitor::TraverseDecl(D);
+    // TODO (Alex): Add location adjustment for ObjCImplDecls.
+    SourceSelectionKind SelectionKind =
----------------
Use FIXME(<user-id>) instead of TODO in LLVM. Here and elsewhere.


================
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:126-129
+    SelectedASTNode Node = std::move(SelectionStack.back());
+    SelectionStack.pop_back();
+    if (SelectionKind != SourceSelectionKind::None || !Node.Children.empty())
+      SelectionStack.back().Children.push_back(std::move(Node));
----------------
This seems to be a recurring pattern. Maybe pull into a function?


Repository:
  rL LLVM

https://reviews.llvm.org/D35012