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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
A few comments based on our discussion.
clangd/ClangdUnit.cpp | ||
---|---|---|
387 ↗ | (On Diff #123046) | example |
416 ↗ | (On Diff #123046) | nit: mention that if a < b, encodeFloat(a) < encodeFloat(b) |
417 ↗ | (On Diff #123046) | I think we'd better have unittest for this function. |
423 ↗ | (On Diff #123046) | 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. |
425 ↗ | (On Diff #123046) | U ^ SignBit |
test/clangd/completion-items-kinds.test | ||
1 ↗ | (On Diff #123046) | an off-topic comment: why not using -pretty in this test? |
Thanks for the quick review, took me a while to get back to this but I do care about it!
clangd/ClangdUnit.cpp | ||
---|---|---|
387 ↗ | (On Diff #123046) | Done, also spelled out more what's going on here. |
417 ↗ | (On Diff #123046) | I'd discussed this briefly with Ilya and we weren't sure it was worthwhile. |
423 ↗ | (On Diff #123046) | 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) |
425 ↗ | (On Diff #123046) | This matches my old explanation but isn't correct for sign-magnitude. |
test/clangd/completion-items-kinds.test | ||
1 ↗ | (On Diff #123046) | 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... |
LGTM
clangd/ClangdUnit.cpp | ||
---|---|---|
425 ↗ | (On Diff #123046) | Well, I see! You are right. The implement is correct here. |