[PATCH] D38835: [refactor] selection: new CodeRangeASTSelection represents a set of selected consecutive statements

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: include/clang/Tooling/Refactoring/ASTSelection.h:74
+/// An AST selection value that corresponds to a selection of a set of
+/// statements that belong to one body of code (like one function).
+///
----------------
I see all your tests are for function body,  does it support other body, i.e. "global namespace", "compound statement"? 

if yes, how about adding more test cases to cover it.

```
// variable in global namespace
int a; // select int.
``` 

```
void f() {
   {
       int a;  // select a.
   }
}
```


================
Comment at: include/clang/Tooling/Refactoring/ASTSelection.h:95
+/// \c SelectedASTNode tree and should not outlive it.
+struct CodeRangeASTSelection {
+  CodeRangeASTSelection(CodeRangeASTSelection &&) = default;
----------------
nit:  instead of a structure, this feels more like a `class`.


================
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:247
+namespace {
+
+struct SelectedNodeWithParents {
----------------
nit: remove the empty line.


================
Comment at: unittests/Tooling/ASTSelectionTest.cpp:685
+)";
+  // Empty range is an invalid code range.
+  findSelectedASTNodesWithRange(
----------------
I think "No selection range" is more precise -- you are passing `None` parameter to the function.

For `empty range`, it should be something like `FileRange{{2, 2}, {2, 2}}`, we can also add a test for it.


================
Comment at: unittests/Tooling/ASTSelectionTest.cpp:688
+      Source, {2, 2}, None,
+      [](SourceRange SelectionRange, Optional<SelectedASTNode> Node) {
+        EXPECT_TRUE(Node);
----------------
I'm a bit confused here, if the selection range is none/empty,  shouldn't `SelectedASTNode` be empty?

If this behavior is intended, I'd suggest documenting it in `findSelectedASTNodesWithRange`.


================
Comment at: unittests/Tooling/ASTSelectionTest.cpp:718
+            isa<TranslationUnitDecl>(Parents[0].get().Node.get<Decl>()));
+        EXPECT_TRUE(isa<FunctionDecl>(Parents[1].get().Node.get<Decl>()));
+        EXPECT_TRUE(isa<CompoundStmt>(Parents[2].get().Node.get<Stmt>()));
----------------
nit: would be clearer to add a comment indicating the corresponding code, the same below.

like

```
EXPECT_TRUE(...); // function f definition
EXPECT_TRUE(...); // function body of function f
```


Repository:
  rL LLVM

https://reviews.llvm.org/D38835