On Fri, 25 Apr 2025 at 09:13, Tomasz Kaminski <tkami...@redhat.com> wrote:
>
>
>
> On Fri, Apr 25, 2025 at 12:19 AM Jonathan Wakely <jwak...@redhat.com> wrote:
>>
>> In r10-452-ge625ccc21a91f3 I noted that we don't have an accessor for
>> invoking _M_impl._M_key_compare in the associative containers. That
>> meant that the static assertions to check for valid comparison functions
>> were squirrelled away in _Rb_tree::_S_key instead. As Jason noted in
>> https://gcc.gnu.org/pipermail/gcc-patches/2025-April/681436.html this
>> means that the static assertions fail later than we'd like.
>>
>> This change adds a new _Rb_tree::_M_key_compare member function which
>> invokes the _M_impl._M_key_compare function object, and then moves the
>> static_assert from _S_key into _M_key_compare.
>>
>> Because the new function is const-qualified, we now treat LWG 2542 as a
>> DR for older standards, requiring the comparison function to be const
>> invocable. Previously we only enforced the LWG 2542 rule for C++17 and
>> later.
>>
>> I did consider deprecating support for comparisons which aren't const
>> invocable, something like this:
>>
>> // Before LWG 2542 it wasn't strictly necessary for _Compare to be
>> // const invocable, if you only used non-const container members.
>> // Define a non-const overload for pre-C++17, deprecated for C++11/14.
>> #if __cplusplus < 201103L
>> bool
>> _M_key_compare(const _Key& __k1, const _Key& __k2)
>> { return _M_impl._M_key_compare(__k1, __k2); }
>> #elif __cplusplus < 201703L
>> template<typename _Key1, typename _Key2>
>> [[__deprecated__("support for comparison functions that are not "
>> "const invocable is deprecated")]]
>> __enable_if_t<
>> __and_<__is_invocable<_Compare&, const _Key1&, const _Key2&>,
>> __not_<__is_invocable<const _Compare&, const _Key1&, const
>> _Key2&>>>::value,
>> bool>
>> _M_key_compare(const _Key1& __k1, const _Key2& __k2)
>> {
>> static_assert(
>> __is_invocable<_Compare&, const _Key&, const _Key&>::value,
>> "comparison object must be invocable with two arguments of key type"
>> );
>> return _M_impl._M_key_compare(__k1, __k2);
>> }
>> #endif // < C++17
>>
>> But I decided that this isn't necessary, because we've been enforcing
>> the C++17 rule since GCC 8.4 and 9.2, and C++17 has been the default
>> since GCC 11.1. Users have had plenty of time to fix their invalid
>> comparison functions.
>
> LGTM with one very small change to comment.
>>
>>
>> libstdc++-v3/ChangeLog:
>>
>> * include/bits/stl_tree.h (_Rb_tree::_M_key_compare): New member
>> function to invoke comparison function.
>> (_Rb_tree): Use new member function instead of accessing the
>> comparison function directly.
>> ---
>>
>> Tested x86_64-linux.
>>
>> libstdc++-v3/include/bits/stl_tree.h | 108 +++++++++++++--------------
>> 1 file changed, 50 insertions(+), 58 deletions(-)
>>
>> diff --git a/libstdc++-v3/include/bits/stl_tree.h
>> b/libstdc++-v3/include/bits/stl_tree.h
>> index 6b35f99a25a..c7352093933 100644
>> --- a/libstdc++-v3/include/bits/stl_tree.h
>> +++ b/libstdc++-v3/include/bits/stl_tree.h
>> @@ -1390,27 +1390,25 @@ namespace __rb_tree
>> _M_end() const _GLIBCXX_NOEXCEPT
>> { return this->_M_impl._M_header._M_base_ptr(); }
>>
>> + // _GLIBCXX_RESOLVE_LIB_DEFECTS
>> + // 2542. Missing const requirements for associative containers
>> + template<typename _Key1, typename _Key2>
>> + bool
>> + _M_key_compare(const _Key1& __k1, const _Key2& __k2) const
>> + {
>> +#if __cplusplus >= 201103L
>> + // Enforce this here with a user-friendly message.
>> + static_assert(
>> + __is_invocable<const _Compare&, const _Key&, const _Key&>::value,
>> + "comparison object must be invocable with two arguments of key
>> type"
>> + );
>> +# endif // C++17
>
> This does not match the condition in if, should be C++11
Oh yes, thanks, I'll fix it.
>>
>> + return _M_impl._M_key_compare(__k1, __k2);
>> + }
>> +
>> static const _Key&
>> _S_key(const _Node& __node)
>> - {
>> -#if __cplusplus >= 201103L
>> - // If we're asking for the key we're presumably using the comparison
>> - // object, and so this is a good place to sanity check it.
>> - static_assert(__is_invocable<_Compare&, const _Key&, const _Key&>{},
>> - "comparison object must be invocable "
>> - "with two arguments of key type");
>> -# if __cplusplus >= 201703L
>> - // _GLIBCXX_RESOLVE_LIB_DEFECTS
>> - // 2542. Missing const requirements for associative containers
>> - if constexpr (__is_invocable<_Compare&, const _Key&, const _Key&>{})
>> - static_assert(
>> - is_invocable_v<const _Compare&, const _Key&, const _Key&>,
>> - "comparison object must be invocable as const");
>> -# endif // C++17
>> -#endif // C++11
>
> I checked that _S_key seems to be mostly passed to __M_key_compare.
> There is one function _M_key or _Auto_node that calls is, but it get passed
> to
> M_get_insert_(hint_)?(equal|uniq)_pos, but that function is called
> _M_comapare.
> In short, yes, this was duplicate of check.
Yes, putting the static_assert checks in _S_key was done because that
function is used whenever we end up invoking the comparison function,
which fixed PR 85965 and still ensured we did the checks in most
situations. The new _M_key_compare function ensures that we check the
static_assert in all paths that need to use the comparison function,
which is probably how I should have fixed PR 85965 in the first place.
