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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
