Re: [PATCH 2/5][_Hashtable] Fix implementation inconsistencies

[Jonathan Wakely <jwakely@redhat.com>]

On Thu, 23 Nov 2023 at 21:59, François Dumont <frs.dumont@gmail.com> wrote:
>
>      libstdc++: [_Hashtable] Fix some implementation inconsistencies
>
>      Get rid of the different usages of the mutable keyword. For
>      _Prime_rehash_policy methods are exported from the library, we need to
>      keep their const qualifier, so adapt implementation to update
> previously
>      mutable member.

If anybody ever declares a const _Prime_rehash_policy and then calls
its _M_next_bkt member or _M_need_rehash member they'll get undefined
behaviour, which seems bad. Probably nobody will ever do that, but if
we just leave the mutable member then that problem doesn't exist.

It would be possible to add non-const overlaods of _M_next_bkt and
_M_need_rehash, and then make the const ones do:

return const_cast<_Prime_rehash_policy*>(this)->_M_next_bkt(n);

or even just define the const symbol as an alias of the non-const
symbol, on targets that support that.  That would still be undefined
if somebody uses a const Prime_rehash_policy object somewhere, but it
would mean the definition of the member functions don't contain nasty
surprises, and new code would call the non-const version, which
doesn't use the unsafe const_cast.

>
>      Remove useless noexcept qualification on _Hashtable _M_bucket_index
> overload.
>      Fix comment to explain that we need the computation of bucket index
> to be
>      noexcept to be able to rehash the container when needed. For Standard
>      instantiations through std::unordered_xxx containers we already force
>      usage of hash code cache when hash functor is not noexcep so it is
> guarantied.
>      The static_assert purpose in _Hashtable on _M_bucket_index is thus
> limited
>      to usages of _Hashtable with exotic _Hashtable_traits.
>
>      libstdc++-v3/ChangeLog:
>
>              * include/bits/hashtable_policy.h
> (_NodeBuilder<>::_S_build): Remove
>              const qualification on _NodeGenerator instance.
> (_ReuseOrAllocNode<>::operator()(_Args&&...)): Remove const qualification.
>              (_ReuseOrAllocNode<>::_M_nodes): Remove mutable.
>              (_Prime_rehash_policy::max_load_factor()): Remove noexcept.

Why?

>              (_Prime_rehash_policy::_M_reset()): Remove noexcept.

Why?

Those functions really are noexcept, right? We should remove noexcept
where it's incorrect or misleading, but here it's OK, isn't it? Or am
I forgetting the problem being solved here?


>              (_Prime_rehash_policy::_M_next_resize): Remove mutable.
>              (_Power2_rehash_policy::_M_next_bkt(size_t)): Remove noexcept.
>              (_Power2_rehash_policy::_M_bkt_for_elements(size_t)):
> Remove noexcept.
>              (_Power2_rehash_policy::_M_neeed_rehash): Remove noexcept.
>              (_Power2_rehash_policy::_M_reset): Remove noexcept.
>              (_Insert_base<>::_M_insert_range): Remove _NodeGetter const
> qualification.
>              (_Hash_code_base<>::_M_bucket_index(const
> _Hash_node_value<>&, size_t)):
>              Simplify noexcept declaration, we already static_assert
> that _RangeHash functor
>              is noexcept.
>              * include/bits/hashtable.h: Rework comments. Remove const
> qualifier on
>              _NodeGenerator& arguments.
>              (_Hashtable<>::_M_bucket_index(const __node_value_type&)):
> Remove useless
>              noexcept qualification.
>              * src/c++11/hashtable_c++0x.cc (_Prime_rehash_policy):
> Workaround
>              _M_next_resize not being mutable anymore.
>
> Tested under Linux x86_64,
>
> ok to commit ?
>
> François