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

[Sam McCall via Phabricator via cfe-commits]

sammccall planned changes to this revision.
sammccall marked 2 inline comments as done.
sammccall added a comment.

Thanks for the comments here and the offline discussion.

The problems identified were:

  - when a node's range is adjusted, then invisible parents (like ExprWithCleanups) don't adjust to match and end up owning tokens.
- it would be nice if CXXConstructExpr claimed the = when copy-assigning, so it would be a go-to-definition target.
  - the FunctionTypeLoc handling is naive, and won't treat `void (*s)()` correctly where the identifier is wrapped in a further typeloc

The best solution to the latter appears to be to lean even more on traversal order and the "already claimed" mechanism: claim the identifier for the VarDecl on the way down, before the children are traversed.

I believe this will eliminate the last case where CXXConstructExpr actually needs special bounds: `Type()` will have Type claimed by a sibling typeloc, `Type t()` will have t claimed by the parent vardecl, etc.
Removing the special bounds will make `=` owned by the CXXConstructExpr, and sidestep the invisible-nodes problem.

And obviously it will simplify the code to have a single range for nodes (once again). So I'll fold it into this patch.



================
Comment at: clangd/Selection.cpp:54
+
+    Ranges.insert(Ranges.end(), NewRanges.begin(), NewRanges.end());
+    llvm::sort(Ranges); // should we merge, too?
----------------
kadircet wrote:
> 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?
I forgot to mention the NewRanges must be disjoint, which (I think) makes this always correct.

Why remove the covered ranges? Just to avoid growing Ranges, since the algorithm is linear in that. Not important, but we're doing the check anyway.

There's no good reason to copy, remove in place, and copy again though, I'll fix that.


================
Comment at: clangd/Selection.cpp:221
+      else if (CCE->getNumArgs() == 1)
+        return computeRanges(CCE->getArg(0));
+      else
----------------
kadircet wrote:
> what happens to parentheses in this case?
There are none; we handled the parens case two lines up :-)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D63760