Page MenuHomePhabricator

[clangd] Deduplicate refs from index for cross-file rename.
ClosedPublic

Authored by hokein on Dec 10 2019, 1:40 PM.

Details

Summary

If the index returns duplicated refs, it will trigger the assertion in
BuildRenameEdit (we expect the processing position is always larger the
the previous one, but it is not true if we have duplication), and also
breaks our heuristics.

This patch make the code robost enough to handle duplications, also
save some cost of redundnat llvm::sort.

Though clangd's index doesn't return duplications, our internal index
kythe will.

Diff Detail

Event Timeline

hokein created this revision.Dec 10 2019, 1:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2019, 1:40 PM

Build result: pass - 60688 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

The fix itself LGTM. Just a few NITs and this is good to go.

clang-tools-extra/clangd/unittests/RenameTests.cpp
34
  • Returning both storage and Ref is quite a complicated interface. Could we rely on the clients to pass strings with URIs instead? It's not a lot of code to create URIs.
  • I think the function name might end up a little confusing when reading the code using it (it certainly did for me). Could we rename it to refWithRange? That seems to describe what this function is doing more accurately.
hokein updated this revision to Diff 233262.Dec 10 2019, 11:58 PM
hokein marked an inline comment as done.

address review comment -- simplify the unittest code.

thanks for the fast review.

clang-tools-extra/clangd/unittests/RenameTests.cpp
34

agree, this is a good point. thanks.

Build result: pass - 60688 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

ilya-biryukov accepted this revision.Dec 11 2019, 12:28 AM

LGTM, thanks!

This revision is now accepted and ready to land.Dec 11 2019, 12:28 AM
This revision was automatically updated to reflect the committed changes.