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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman 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).
+///
----------------
hokein wrote:
> 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.
>    }
> }
> ```
Yes, I added a couple that you suggested.


================
Comment at: unittests/Tooling/ASTSelectionTest.cpp:688
+      Source, {2, 2}, None,
+      [](SourceRange SelectionRange, Optional<SelectedASTNode> Node) {
+        EXPECT_TRUE(Node);
----------------
hokein wrote:
> 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`.
No, the AST selection will work even with a cursor location, hence it won't be empty.

However, the CodeRangeASTSelection requires a non-empty selection range, so it won't work with just cursors.


Repository:
  rL LLVM

https://reviews.llvm.org/D38835