[PATCH] D89579: [clangd][ObjC] Support nullability annotations

[David Goldman via Phabricator via cfe-commits]

dgoldman added inline comments.


================
Comment at: clang-tools-extra/clangd/Selection.cpp:611
+      // robust.
+      return false;
     if (!SelChecker.mayHit(S)) {
----------------
sammccall wrote:
> I'm not sure we actually want to *do it* at this point, as you're seeing we may have some unintended consequences.
Ah, I think I found the cause: claimRange will claim tokens for the TypeLoc even if the TypeLoc is unselected. So the VarDecl later on appears to be completely selected since the `int` token has been claimed and `claimRange` doesn't check the status of the children.

Before:



> Built preamble of size 206192 for file /clangd-test/TestTU.cpp version null
> Computing selection for </clangd-test/TestTU.cpp:1:6, col:11>
>  push: VarDecl 
>   hit selection: </clangd-test/TestTU.cpp:1:6>
>   push: NestedNameSpecifierLoc 
>   pop: NestedNameSpecifierLoc  selected: 3
>   skip: BuiltinTypeLoc 
>    skipped range = </clangd-test/TestTU.cpp:1:2>
>   push: IntegerLiteral 
>    hit selection: </clangd-test/TestTU.cpp:1:10>
>   pop: IntegerLiteral  selected: 2
>   hit selection: </clangd-test/TestTU.cpp:1:2, col:10>
>  pop: VarDecl  selected: 1
> Built selection tree
>  TranslationUnitDecl 
>   .VarDecl int x = 0
>     *IntegerLiteral 0




After:


> Built preamble of size 206192 for file /clangd-test/TestTU.cpp version null
> Computing selection for </clangd-test/TestTU.cpp:1:6, col:11>
>  push: VarDecl
>   hit selection: </clangd-test/TestTU.cpp:1:6>
>   push: NestedNameSpecifierLoc
>   pop: NestedNameSpecifierLoc  selected: 3
>   push: BuiltinTypeLoc
>   pop: BuiltinTypeLoc  selected: 0
>   push: IntegerLiteral
>    hit selection: </clangd-test/TestTU.cpp:1:10>
>   pop: IntegerLiteral  selected: 2
>   hit selection: </clangd-test/TestTU.cpp:1:2, col:10>
>  pop: VarDecl  selected: 2
> Built selection tree
>  TranslationUnitDecl
>   *VarDecl int x = 0
>     *IntegerLiteral 0

Like you said probably worth fixing in a follow up, and I should revert to my previous code?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89579