Re: [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator<I, C>>

[Jonathan Wakely <jwakely@redhat.com>]

On Tue, 14 Dec 2021 at 06:53, François Dumont wrote:

> Hi
>
>      Any conclusion regarding this thread ?
>
> François
>
>
> On 06/10/21 7:25 pm, François Dumont wrote:
> > I forgot to ask if with this patch this overload:
> >
> >   template<typename _Ptr, typename... _None>
> >     constexpr auto
> >     __to_address(const _Ptr& __ptr, _None...) noexcept
> >     {
> >       if constexpr (is_base_of_v<__gnu_debug::_Safe_iterator_base, _Ptr>)
> >     return std::__to_address(__ptr.base().operator->());
> >       else
> >     return std::__to_address(__ptr.operator->());
> >     }
> >
> > should be removed ?
>
>
No, definitely not.

That is the default overload for types that do not have a
pointer_traits::to_address specialization. If you remove it, __to_address
won't work for fancy pointers or any other pointer-like types. That would
completely break it.

The purpose of C++20's std::to_address is to get a real pointer from a
pointer-like type. Using it with iterators is not the primary use case, but
it needs to work with contiguous iterators because those are pointer-like.
I made it work correctly with __normal_iterator because that was necessary
to support the uses of std::__to_address in <regex> and <filesystem>, but I
removed those uses in:

https://gcc.gnu.org/g:247bac507e63b32d4dc23ef1c55f300aafea24c6
https://gcc.gnu.org/g:b83b810ac440f72e7551b6496539e60ac30c0d8a

So now we don't really need the C++17 version of std::__to_address to work
with __normal_iterator at all.

I think it's OK to add the overload for __normal_iterator though, but only
for C++11/14/17, because the default std::__to_address handles
__normal_iterator correctly in C++20.


> Or perhaps just the _Safe_iterator_base branch in it ?
>

Yes, you can just remove that branch, because your new overload handles it.


>

> > On 06/10/21 7:18 pm, François Dumont wrote:
> >> Here is another proposal with the __to_address overload.
> >>
> >> I preferred to let it open to any kind of __normal_iterator
> >> instantiation cause afaics std::vector supports fancy pointer types.
> >> It is better if __to_address works fine also in this case, no ?
>

 If we intend to support that, then we should verify it in the testsuite,
using __gnu_test::CustomPointerAlloc.


>>     libstdc++: Overload std::__to_address for
> >> __gnu_cxx::__normal_iterator.
> >>
> >>     Prefer to overload __to_address to partially specialize
> >> std::pointer_traits because
> >>     std::pointer_traits would be mostly useless. In the case of
> >> __gnu_debug::_Safe_iterator
> >>     the to_pointer method is even impossible to implement correctly
> >> because we are missing
> >>     the parent container to associate the iterator to.
>

To record additional rationale in the git history, please add that the
partial specialization of pointer_traits<__normal_iterator<P, C>> fails to
rebind C, so you get incorrect types like __normal_iterator<long*,
vector<int>>.


>>
> >>     libstdc++-v3/ChangeLog:
> >>
> >>             * include/bits/stl_iterator.h
> >> (std::pointer_traits<__gnu_cxx::__normal_iterator<>>): Remove.
>

OK to remove this (it's broken anyway).

>>             (std::__to_address(const
> >> __gnu_cxx::__normal_iterator<>&)): New.
>

Please make this only defined for C++11, 14 and 17.


> >>             * include/debug/safe_iterator.h
> >>             (std::__to_address(const
> >> __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<>,
> >> _Sequence>&)):
> >>             New.
>

OK to add this (including for C++20), and remove the _Safe_iterator branch
from the C++20 std::__to_address in <bits/ptr_traits.h>.

I think this new overload could return
std::__to_address(__it.base().base()) though. That saves a function call,
by going directly to the value stored in the __normal_iterator.



> >>             * testsuite/24_iterators/normal_iterator/to_address.cc:
> >> Add check on std::vector::iterator
> >>             to validate both __gnu_cxx::__normal_iterator<>
> >> __to_address overload in normal mode and the
>

Add similar checks for vector<int, __gnu_test::CustomPointerAlloc<int>>.

OK with those changes, thanks.