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

Mon Sep 19 10:07:34 PDT 2022

ldionne added a comment.

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

> As mentioned in PR56606, CityHash seems not being actively maintained anymore.  
> Do we want to change our hash implementation to FarmHash or other hash functions?
>
> Not sure whether this is a good place for discussion, though.

I think this is really tricky to change, since we need to remain ABI compatible with code previously compiled. In other words, let's say an already compiled library creates an `unordered_map` with the old hash function and passes that to a program compiled with a new libc++ (with the new hashing function): the newly compiled program would have to be able to manipulate the old `unordered_map`. It being slower is probably reasonable, but it would have to function correctly.

We could introduce a new hashing algorithm in our unstable ABI, but I think we should still fix this issue with our existing CityHash algorithm.

@oToToT are you able to add a test that would fail with our current (buggy) implementation? https://github.com/llvm/llvm-project/issues/56606 seems to have a reproducer.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134124