[PATCH] D40089: [clangd] Make completion scores use 0-1 floats internally.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 4 inline comments as done.
sammccall added a comment.

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



================
Comment at: clangd/ClangdUnit.cpp:387
 
   std::string sortText() const {
     std::string S, NameStorage;
----------------
hokein wrote:
> example
Done, also spelled out more what's going on here.


================
Comment at: clangd/ClangdUnit.cpp:417
+  // Produces an integer that sorts in the same order as F.
+  static uint32_t encodeFloat(float F) {
+    // IEEE 754 floats compare like 2s complement integers.
----------------
hokein wrote:
> I think we'd better have unittest for this function.
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?


================
Comment at: clangd/ClangdUnit.cpp:423
+    memcpy(&U, &F, sizeof(float));
+    // Convert U from 2s complement to unsigned, preserving order.
+    const uint32_t SignBit = ~(~uint32_t{0} >> 1);
----------------
hokein wrote:
> 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.
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)


================
Comment at: clangd/ClangdUnit.cpp:425
+    const uint32_t SignBit = ~(~uint32_t{0} >> 1);
+    return U & SignBit ? 0 - U : U + SignBit;
+  }
----------------
hokein wrote:
> U ^ SignBit
This matches my old explanation but isn't correct for sign-magnitude.


================
Comment at: test/clangd/completion-items-kinds.test:1
 # RUN: clangd -enable-snippets -run-synchronously < %s | FileCheck %s
 # It is absolutely vital that this file has CRLF line endings.
----------------
hokein wrote:
> an off-topic comment: why not using `-pretty` in this test?
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...


https://reviews.llvm.org/D40089