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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

One request, when you address a review comment, can you check the "done" button this makes reviewing easier.

I had a look at the changes and I'm wondering about the impact on the size of the binary. You add one template argument to "only" adjust the `__rehash` function. This means the `__hash_table` for `unordered_map<K, V>` and `unordered_multimap<K, V>` will be different after this change. Could you provide some numbers regarding how much the binary will grow when an applications uses both `unordered_map<K, V>` and unordered_multimap<K, V>`?



================
Comment at: libcxx/include/__hash_table:2284
                         __next_pointer __np = __cp;
-                        for (; __np->__next_ != nullptr &&
-                               key_eq()(__cp->__upcast()->__value_,
-                                        __np->__next_->__upcast()->__value_);
-                                                           __np = __np->__next_)
-                            ;
+			if (!_UniqueKeys::value) {
+                            for (; __np->__next_ != nullptr &&
----------------
philnik wrote:
> philnik wrote:
> > jloser wrote:
> > > Question: what's the libc++ stance for something like this which could be `if constexpr` in C++17 or later? Is it worth writing
> > > 
> > > ```
> > > #if _LIBCPP_STD_VER >= 17
> > > if constexpr
> > > #else
> > > if
> > > #endif
> > > (!UniqueKeys::Value) {
> > > ```
> > > 
> > > I suspect the answer is just writing `if` for all standards modes and just letting the compiler realize it's always a constant value now up-front.
> > It's definitely not worth it. This makes the code a lot less readable for a minor to non-existent improvement in compile times.
> Could you indent this properly?
> Question: what's the libc++ stance for something like this which could be `if constexpr` in C++17 or later? Is it worth writing
> 
> ```
> #if _LIBCPP_STD_VER >= 17
> if constexpr
> #else
> if
> #endif
> (!UniqueKeys::Value) {
> ```
> 
> I suspect the answer is just writing `if` for all standards modes and just letting the compiler realize it's always a constant value now up-front.

There's no need for the `#if LIBCPP_STD_VER >= 17`, `if _LIBCPP_CONSTEXPR_AFTER_CXX14 (!UniqueKeys::Value) {` should work.


================
Comment at: libcxx/include/ext/hash_map:490
     typedef std::__hash_table<__value_type, __hasher,
-                         __key_equal,  __allocator_type>   __table;
+                         __key_equal,  __allocator_type, std::__hash_table_unique_keys<true> >   __table;
 
----------------
There's no need for `std::` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128021