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