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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Also there were some offline discussions around, handling of "invisible nodes"(e.g, `ExprWithCleanups`) and other types of typelocs like ParenTypeLocs and owner of '=' sign in copy/move assignment constructors



================
Comment at: clangd/Selection.cpp:54
+
+    Ranges.insert(Ranges.end(), NewRanges.begin(), NewRanges.end());
+    llvm::sort(Ranges); // should we merge, too?
----------------
assume we have inserted the range `[1,5]` and then tried inserting `{[1,5], [2,3]}`.
In that case `IsCovered` would return `false` for  `[2,3]`. And `add` would return `true`, but all of the newranges were contained in the `RangeSet`

Also if we are going to store possibly-overlapping `OffsetRanges` why are we trying to remove those?


================
Comment at: clangd/Selection.cpp:136
   using Base = RecursiveASTVisitor<SelectionVisitor>;
+  using Ranges = SmallVector<SourceRange, 1>;
+  using OffsetRange = std::pair<unsigned, unsigned>;
----------------
what about moving this to top of the file?


================
Comment at: clangd/Selection.cpp:137
+  using Ranges = SmallVector<SourceRange, 1>;
+  using OffsetRange = std::pair<unsigned, unsigned>;
+
----------------
already defined at top


================
Comment at: clangd/Selection.cpp:221
+      else if (CCE->getNumArgs() == 1)
+        return computeRanges(CCE->getArg(0));
+      else
----------------
what happens to parentheses in this case?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D63760