On Wed, 15 Dec 2021 at 21:16, François Dumont <frs.dum...@gmail.com> wrote:
> Here is what I eventually would like to commit. > > I was not able to remove the _Safe_iterator_base branch in ptr_traits.h. > When adding the _Safe_iterator overload in C++20 and removing the branch > the 20_util/to_address/debug.cc test started to fail because it was not > calling my overload. I tried to declare the overload in ptr_traits.h > directly so it is known at the time it is used in std::to_address but then > it failed to match it with the implementation in safe_iterator.h. The > declaration was not easy to do and I guess I had it wrong. > > But it does not matter cause I think this version is the simplest one (as > it does not change a lot of code). > > 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. Moreover partial > specialization of > pointer_traits<__normal_iterator<P, C>> fails to rebind C, so you get > incorrect types > like __normal_iterator<long*, vector<int>>. In the case of > __gnu_debug::_Safe_iterator > the to_pointer method is impossible to implement correctly because we > are missing > the parent container to associate the iterator to. > > libstdc++-v3/ChangeLog: > > * include/bits/stl_iterator.h > (std::pointer_traits<__gnu_cxx::__normal_iterator<>>): Remove. > (std::__to_address(const __gnu_cxx::__normal_iterator<>&)): > New for C++11 to C++17. > * include/debug/safe_iterator.h > (std::__to_address(const > __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<>, _Sequence>&)): > New for C++11 to C++17. > * 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 > __gnu_debug::_Safe_iterator in _GLIBCXX_DEBUG mode. > > Tested under Linux x86_64 normal and _GLIBCXX_DEBUG modes for > C++11/C++14/C++17/C++20. > > Ok to commit ? > OK, thanks! > François > > > On 14/12/21 2:12 pm, Jonathan Wakely wrote: > > 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. > > > > >