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

["François Dumont" <frs.dumont@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6971 bytes --]

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 ?

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:247bac507e63b32d4dc23ef1c55f300aafea24c6>
> https://gcc.gnu.org/g:b83b810ac440f72e7551b6496539e60ac30c0d8a 
> <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.



[-- Attachment #2: to_address.patch --]
[-- Type: text/x-patch, Size: 3876 bytes --]

diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
index 6bd860b803e..ac9342112f4 100644
--- a/libstdc++-v3/include/bits/stl_iterator.h
+++ b/libstdc++-v3/include/bits/stl_iterator.h
@@ -1349,32 +1349,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
 #if __cplusplus >= 201103L
 
-  // Need to specialize pointer_traits because the primary template will
-  // deduce element_type of __normal_iterator<T*, C> as T* rather than T.
+#if __cplusplus <= 201703L
+  // Need to overload __to_address because the pointer_traits primary template
+  // will deduce element_type of __normal_iterator<T*, C> as T* rather than T.
   template<typename _Iterator, typename _Container>
-    struct pointer_traits<__gnu_cxx::__normal_iterator<_Iterator, _Container>>
-    {
-    private:
-      using _Base = pointer_traits<_Iterator>;
-
-    public:
-      using element_type = typename _Base::element_type;
-      using pointer = __gnu_cxx::__normal_iterator<_Iterator, _Container>;
-      using difference_type = typename _Base::difference_type;
-
-      template<typename _Tp>
-	using rebind = __gnu_cxx::__normal_iterator<_Tp, _Container>;
-
-      static pointer
-      pointer_to(element_type& __e) noexcept
-      { return pointer(_Base::pointer_to(__e)); }
-
-#if __cplusplus >= 202002L
-      static element_type*
-      to_address(pointer __p) noexcept
-      { return __p.base(); }
+    constexpr auto
+    __to_address(const __gnu_cxx::__normal_iterator<_Iterator,
+						    _Container>& __it) noexcept
+    -> decltype(std::__to_address(__it.base()))
+    { return std::__to_address(__it.base()); }
 #endif
-    };
 
   /**
    * @addtogroup iterators
diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h
index 5584d06de5a..9c821c82e17 100644
--- a/libstdc++-v3/include/debug/safe_iterator.h
+++ b/libstdc++-v3/include/debug/safe_iterator.h
@@ -1013,6 +1013,23 @@ namespace __gnu_debug
 
 } // namespace __gnu_debug
 
+#if __cplusplus >= 201103L && __cplusplus <= 201703L
+namespace std _GLIBCXX_VISIBILITY(default)
+{
+_GLIBCXX_BEGIN_NAMESPACE_VERSION
+
+  template<typename _Iterator, typename _Container, typename _Sequence>
+    constexpr auto
+    __to_address(const __gnu_debug::_Safe_iterator<
+		 __gnu_cxx::__normal_iterator<_Iterator, _Container>,
+		 _Sequence>& __it) noexcept
+    -> decltype(std::__to_address(__it.base().base()))
+    { return std::__to_address(__it.base().base()); }
+
+_GLIBCXX_END_NAMESPACE_VERSION
+}
+#endif
+
 #undef _GLIBCXX_DEBUG_VERIFY_DIST_OPERANDS
 #undef _GLIBCXX_DEBUG_VERIFY_REL_OPERANDS
 #undef _GLIBCXX_DEBUG_VERIFY_EQ_OPERANDS
diff --git a/libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc b/libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc
index 510d627435f..6afc6540609 100644
--- a/libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc
+++ b/libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc
@@ -1,6 +1,19 @@
 // { dg-do compile { target { c++11 } } }
 #include <string>
+#include <vector>
 #include <memory>
 
-char* p = std::__to_address(std::string("1").begin());
-const char* q = std::__to_address(std::string("2").cbegin());
+#include <testsuite_allocator.h>
+
+char* p __attribute__((unused))
+  = std::__to_address(std::string("1").begin());
+const char* q __attribute__((unused))
+  = std::__to_address(std::string("2").cbegin());
+int* r __attribute__((unused))
+  = std::__to_address(std::vector<int>(1, 1).begin());
+const int* s __attribute__((unused))
+  = std::__to_address(std::vector<int>(1, 1).cbegin());
+int* t __attribute__((unused))
+  = std::__to_address(std::vector<int, __gnu_test::CustomPointerAlloc<int>>(1, 1).begin());
+const int* u __attribute__((unused))
+  = std::__to_address(std::vector<int, __gnu_test::CustomPointerAlloc<int>>(1, 1).cbegin());