[libcxx-commits] [PATCH] D134124: [libc++] Fix wrong implementation of CityHash

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

In D134124#3801752 <https://reviews.llvm.org/D134124#3801752>, @oToToT wrote:

> Besides using a new hash algorithm, would we want to upgrade our CityHash version to v1.1 [1]?
> The v1.1 version claims to have a better hash quality. (I haven't test it though)
> And, IIUC, upgrading to v1.1 wouldn't change the ABI.
>
> [1] https://github.com/google/cityhash/commit/0469694b4609253742bcd3008c0b3f3ba9fcd9ba

Yes, that might be interesting, however we'd need to analyze whether we still find elements in an `unordered_map` when we change the hash in that way. Technically, changing the hash means that we'd violate http://eel.is/c++draft/hash.requirements#2: if one part of the program has the old CityHash implementation inlined and another part of the program has the new CityHash implementation, then all evaluations of `h(k)` might not yield the same result. We'd have to analyze whether that is benign UB or actual UB that's going to cause crashes and whatnot.

In all cases, we could find the best hash implementation out there and use that for ABI v2.



================
Comment at: libcxx/test/libcxx/utilities/utility/__murmur2_or_cityhash.pass.cpp:9
+
+// Test the CityHash implementation is correct.
+
----------------
I assume this test fails without your change?

Edit: I checked locally and it does, great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134124