[PATCH] D63760: [clangd] Address limitations in SelectionTree:

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!



================
Comment at: clangd/Selection.cpp:45
+      if (R.first > Begin)
+        return false; // [R.First, Begin) is not covered.
+      if (Begin < R.second)
----------------
I suppose comment should be `[Begin, R.first)`. Could you also change the order within condition?


================
Comment at: clangd/Selection.cpp:48
+        Begin = R.second; // Prefix is covered, truncate the range.
+      if (Begin >= End)
+        return true;
----------------
nit: maybe move this check into previous condition? i.e:
```
if (Begin < R.second) {
      Begin = R.second; // Prefix is covered, truncate the range.
      if (Begin >= End)
        return true;
}


================
Comment at: clangd/Selection.cpp:236
   // This runs for every node in the AST, and must be fast in common cases.
   // This is called from pop(), so we can take children into account.
+  SelectionTree::Selection claimRange(SourceRange S) {
----------------
also called from push() now


================
Comment at: clangd/unittests/SelectionTests.cpp:170
+            struct S { S(const char*); };
+            S [[s ^= "foo"]];
+          )cpp",
----------------
could you also add a test case with cursor on identifier(i.e, `s`)


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63760/new/

https://reviews.llvm.org/D63760