[PATCH] D116218: [clangd] Fix selection on multi-dimensional array.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/Selection.cpp:526
+  // the traversal order of SizeExpr and ElementTypeLoc, which gives a chance
+  // for the SizeExpr to claim its tokens.
+  bool TraverseConstantArrayTypeLoc(ConstantArrayTypeLoc X) {
----------------
sammccall wrote:
> I don't think this is a complete solution: won't the inner ArrayTypeLoc still end up owning both sets of brackets?
> 
> I think maybe a better solution is making getSourceRange(ArrayTypeLoc) return only ATL.getBracketRange(), and then modify canSafelySkipNode to to avoid pruning based on this small range.
> 
> This is vaguely similar to how DeclTypeLoc works today (though in that case the reduced range is the one reported by the AST).
> won't the inner ArrayTypeLoc still end up owning both sets of brackets?

yes, unfortunately, the inner ATL still owns both sets of brackets, that means go-to-def (etc) won't work on an overloaded subscript operator [], I don't have a better solution to fix that. I guess it is ok to live with that as it might be a rare case in practice.

> I think maybe a better solution is making getSourceRange(ArrayTypeLoc) return only ATL.getBracketRange(), and then modify canSafelySkipNode to to avoid pruning based on this small range.

yeah, indeed I tried this approach before coming up the current approach, and I didn't find a satisfied solution (BracketRange is not enough, otherwise we will lose the array type `int`). 


```
 // int array[Size][100];
 // ~~~      [-2 -]~~~~~  ConstantArrayTypeLoc int[Size][100]
 // [1]            [-3-]  |-ConstantArrayTypeLoc int[100]

```
Ideally, the inner ATL should own tokens `int` ([1]) and tokens `[100]` ([3]). And in the `canSafelySkipNode`, we want to skip the inner ATL if the cursor is in the source range of ` array[Size]`, it seems quite tricky to get the source range (the range of `int` [1] in particular). Of course we could use some lexer hacks or call `ATL.getElementLoc()` recursively until we hit a non-array type loc (it can easily go up to  O(n^2) for super-multi-dimensional arrays).




================
Comment at: clang-tools-extra/clangd/Selection.cpp:527
+  // for the SizeExpr to claim its tokens.
+  bool TraverseConstantArrayTypeLoc(ConstantArrayTypeLoc X) {
+    if (!Base::TraverseStmt(X.getSizeExpr()))
----------------
sammccall wrote:
> ConstantArrayType isn't the only kind of array, see the other subclasses of ArrayType
oh, right. There are three others, we could do similar for them. Will do that if we agree on the current approach. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116218