This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Make completion scores use 0-1 floats internally.
ClosedPublic

Authored by sammccall on Nov 15 2017, 9:40 AM.

Details

Summary

This scale is much easier to mix with other signals, such as fuzzy match strength.
Mostly NFC, but it does reorder some low-priority items that get folded together at a score of 0 (see completion-qualifiers.test).
Removed the exact sortText from the testcases, because it's the ranking that we want to test.

Event Timeline

sammccall created this revision.Nov 15 2017, 9:40 AM
hokein edited edge metadata.Nov 17 2017, 7:18 AM

A few comments based on our discussion.

clangd/ClangdUnit.cpp
390–391

example

419

nit: mention that if a < b, encodeFloat(a) < encodeFloat(b)

420

I think we'd better have unittest for this function.

426

Would be nice to explain more what we do here: we are trying to remap from the signed range [INT_MIN, INT_MAX] to the unsigned range [0, UINT_MAX], so that we can compare them via string literal comparison.

428

U ^ SignBit

test/clangd/completion-items-kinds.test
1

an off-topic comment: why not using -pretty in this test?

sammccall marked 4 inline comments as done.Nov 23 2017, 1:27 AM

Thanks for the quick review, took me a while to get back to this but I do care about it!

clangd/ClangdUnit.cpp
390–391

Done, also spelled out more what's going on here.

420

I'd discussed this briefly with Ilya and we weren't sure it was worthwhile.
This is a pretty deep implementation detail - would you expose the function from ClangdUnit? Protocol?

426

Turns out the explanation was slightly wrong (but the implementation right, because I lifted it from a trusted source :-)

Spelled this out some more. (Also sign-magnitude is a bit easier to understand that 2s complement)

428

This matches my old explanation but isn't correct for sign-magnitude.

test/clangd/completion-items-kinds.test
1

Because of the old check-dag when we didn't want to assert the order.

It's still clumsy to express incomplete multiline assertions due to FileCheck limitations though. Given the amount of test merge conflicts i'm looking at, I'd rather not change this in this patch...

sammccall updated this revision to Diff 124044.Nov 23 2017, 1:29 AM
sammccall marked 3 inline comments as done.

address review comments

hokein accepted this revision.Nov 23 2017, 6:51 AM

LGTM

clangd/ClangdUnit.cpp
428

Well, I see! You are right. The implement is correct here.

This revision is now accepted and ready to land.Nov 23 2017, 6:51 AM
This revision was automatically updated to reflect the committed changes.