[libcxx-commits] [PATCH] D128021: [libc++] Don't call key_eq in unordered_map/set rehashing routine

[Ivan via Phabricator via libcxx-commits]

itrofimow added a comment.

In D128021#3613221 <https://reviews.llvm.org/D128021#3613221>, @Mordante wrote:

> I had a look at the code and I wonder whether a different approach might be better. Instead of adding a template argument we change `__rehash` to two function `__rehash_unique` and `__rehash_multi`. This seems the approach already taken for other functions in the `__hash_table` class.
>
> This means the public functions `rehash` and `resize` also need two versions. The other functions calling `rehash` are already specific for unique and multi. I think we can keep the unique key argument on these rehash functions. So I would suggest something along the lines of.
>
>   // These were rehash, keep them public and adjust the callers
>   _LIBCPP_INLINE_VISIBILITY void __rehash_unique(size_type __n) { __rehash<true>(__n); }
>   _LIBCPP_INLINE_VISIBILITY void __rehash_multi(size_type __n) { __rehash<false>(__n); }
>   
>   // These were reserve, keep them public and adjust the callers
>   _LIBCPP_INLINE_VISIBILITY void __reserve_unique(size_type __n)
>         {__rehash_unique(static_cast<size_type>(ceil(__n / max_load_factor())));}                                              
>   _LIBCPP_INLINE_VISIBILITY void __reserve_multi(size_type __n)
>         {__rehash_multi(static_cast<size_type>(ceil(__n / max_load_factor())));}
>   
>   
>   // rename __rehash to __do_rehash and make it templated
>   template<class _UniqueKeys>
>   void __do_rehash(size_type __n);
>   
>   // rename rehash to __rehash, make it private and make it templated
>   template<class _UniqueKeys>
>   void __rehash(size_type __n);
>
> Then we can keep the templated rehashing you added but only have the template arguments on these two function instead of the entire class.
>
> WDYT?

If binary size is a concern i definitely like this approach more


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128021