[PATCH] D71598: [clangd] Filter implicit references from index while renaming

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:591
 // Details:
-//  - lex the draft code to get all rename candidates, this yields a superset
-//    of candidates.
+//  - lex the draft code to get all rename candidates, this yields a set of
+//    candidates. It may be a superset of candidates returned from the index
----------------
kadircet wrote:
> i believe lexing might yield a superset even if index is up-to-date,
> 
> ```
> void foo() { int bar; }
> void baz() { int ba^r; }
> 
> ```
Good point. Any local variable (IIRC we don't store local variables in index, so it's not there) having the same identifier as the renamed symbol might cause the indexed ranges to be a subset of lexed ranges.

However, 

> i believe lexing might yield a superset even if index is up-to-date
> exactly for the same reason above, it might not be a subset even in those circumstances.

I tried to describe the case when lexer will return a _subset_ of indexed results, which happens in practice. Is the suggestion to change the comments you think are misleading?


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:597
+//    references to the renamed entity have been removed.
+//  - if the lexed ranges are subset of index candidates, try to filter the
+//    results from index by exactly matching existing ranges from lexer.
----------------
kadircet wrote:
> i believe we should do that even if it is not a subset.
Maybe my understanding of `getMappedRanges` is incorrect, but I suppose this will happen in case indexed ranges are subset of lexed ranges in some form (i.e. no need to find exact matches explicitly). Am I missing something?


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

https://reviews.llvm.org/D71598