This is an archive of the discontinued LLVM Phabricator instance.

[clangd] NFC: Use TopN instead of std::priority_queue
ClosedPublic

Authored by kbobyrev on Sep 5 2018, 2:59 AM.

Details

Summary

Quality.cpp defines a structure for convenient storage of Top N items, it should be used instead of the std::priority_queue with slightly obscure semantics.

This patch does not affect functionality.

Diff Detail

Event Timeline

kbobyrev created this revision.Sep 5 2018, 2:59 AM
sammccall accepted this revision.Sep 5 2018, 12:57 PM

Thanks for cleaning this up!

I believe this will result in the results from MemIndex being returned in best -> worst order, rather than worst -> best.
The contract says callers shouldn't rely on the order so it should still be NFC overall.
It's likely to make your index explorer more useful though :-)

clang-tools-extra/clangd/index/MemIndex.cpp
40–41

this looks more like generic documentation for Top::push.
I'd suggest instead adding a line comment when it's true:
More = true; // We discarded something.

This revision is now accepted and ready to land.Sep 5 2018, 12:57 PM
kbobyrev updated this revision to Diff 164200.Sep 6 2018, 6:11 AM
kbobyrev marked an inline comment as done.

Thanks for cleaning this up!

I believe this will result in the results from MemIndex being returned in best -> worst order, rather than worst -> best.
The contract says callers shouldn't rely on the order so it should still be NFC overall.
It's likely to make your index explorer more useful though :-)

Right. The index explorer would probably still have to re-order elements (so that it actually doesn't rely on implementation details and implicit assumptions). For the completion part, IIUC the complete ranking would rearrange items anyway.

kbobyrev retitled this revision from [clangd] Use TopN instead of std::priority_queue to [clangd] NFC: Use TopN instead of std::priority_queue.Sep 6 2018, 6:14 AM
This revision was automatically updated to reflect the committed changes.