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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:431
+// REQUIRES: Indexed and Lexed are sorted.
+// REQUIRES: Indexed and Lexed are sorted.
+llvm::Optional<std::vector<Range>> filterIndexResults(ArrayRef<Range> Indexed,
----------------
duplication


================
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
----------------
i believe lexing might yield a superset even if index is up-to-date,

```
void foo() { int bar; }
void baz() { int ba^r; }

```


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:595
+//    It may also be a subset of candidates from the index in case when index
+//    returns some incorrect results (such as implicit references) or when some
+//    references to the renamed entity have been removed.
----------------
exactly for the same reason above, it might not be a subset even in those circumstances.


================
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.
----------------
i believe we should do that even if it is not a subset.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:626
   assert(std::is_sorted(Lexed.begin(), Lexed.end()));
+  +assert(Indexed.size() <= Lexed.size());
 
----------------
`+`


================
Comment at: clang-tools-extra/clangd/refactor/Rename.h:80
 /// REQUIRED: Indexed and Lexed are sorted.
+/// REQUIRED: Indexed.size() > Lexed.size().
 llvm::Optional<std::vector<Range>> getMappedRanges(ArrayRef<Range> Indexed,
----------------
assertion below says
`assert(Indexed.size() <= Lexed.size());`


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

https://reviews.llvm.org/D71598