[libc-commits] [PATCH] D136487: [libc.search] [PATCH 1/6] add wyhash as a header library

[Michael Jones via Phabricator via libc-commits]

michaelrj added a comment.

I've done a general style review



================
Comment at: libc/src/__support/wyhash.h:15
+// WyHash comes from https://github.com/wangyi-fudan/wyhash/ (which has been
+// release into public domain). It is also the default hash function
+// of Go, Nim and Zig.
----------------
nit: "released"


================
Comment at: libc/src/__support/wyhash.h:23
+// to effectively locate candidate entries without too many false positives.
+namespace __llvm_libc::cpp::wyhash {
+static constexpr inline uint64_t DEFAULT_SECRET[4] = {
----------------
the `cpp` namespace is for the CPP subdirectory. This should be `internal` instead.


================
Comment at: libc/src/__support/wyhash.h:25-26
+static constexpr inline uint64_t DEFAULT_SECRET[4] = {
+    0xa0761d6478bd642full, 0xe7037ed1a0b428dbull, 0x8ebc6af09c88c6e3ull,
+    0x589965cc75374cc3ull};
+template <bool EntropyProtection> class WyHash {
----------------
add a comment explaining how you calculated these constants.


================
Comment at: libc/src/__support/wyhash.h:45-46
+    uint64_t value;
+    inline_memcpy(reinterpret_cast<char *>(&value),
+                  reinterpret_cast<const char *>(p), N);
+    return Endian::to_little_endian(value);
----------------
this should be a loop with shifts instead of memcpy, which should also allow you to avoid the endianness call below.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136487/new/

https://reviews.llvm.org/D136487