| Message ID | YVNsDz9xJszYeoNl@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [committed] libstdc++: Specialize std::pointer_traits<__normal_iterator<I,C>> | expand |
Here is the _Safe_iterator one. Doing so I noticed that pointer_traits rebind for __normal_iterator was wrong and added tests on it. For _Safe_iterator maybe I should specialize only when instantiated with __normal_iterator ? Or maybe limit to random_access_iterator_tag ? Whatever the pointer_to implementation is problematic, we can only produce singular iterator as I did ifor now. François On 28/09/21 9:25 pm, Jonathan Wakely via Libstdc++ wrote: > This allows std::__to_address to be used with __normal_iterator in > C++11/14/17 modes. Without the partial specialization the deduced > pointer_traits::element_type is incorrect, and so the return type of > __to_address is wrong. > > A similar partial specialization is probably needed for > __gnu_debug::_Safe_iterator. > > Signed-off-by: Jonathan Wakely <jwakely@redhat.com> > > libstdc++-v3/ChangeLog: > > * include/bits/stl_iterator.h (pointer_traits): Define partial > specialization for __normal_iterator. > * testsuite/24_iterators/normal_iterator/to_address.cc: New test. > > Tested x86_64-linux. Committed to trunk. > diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h index 004d767224d..f7e851718c1 100644 --- a/libstdc++-v3/include/bits/stl_iterator.h +++ b/libstdc++-v3/include/bits/stl_iterator.h @@ -1294,13 +1294,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION private: using _Base = pointer_traits<_Iterator>; + template<typename _Tp> + using __base_rebind = typename _Base::template rebind<_Tp>; + 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>; + using rebind = + __gnu_cxx::__normal_iterator<__base_rebind<_Tp>, _Container>; static pointer pointer_to(element_type& __e) noexcept diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h index 5584d06de5a..5461d2b342f 100644 --- a/libstdc++-v3/include/debug/safe_iterator.h +++ b/libstdc++-v3/include/debug/safe_iterator.h @@ -1013,6 +1013,44 @@ namespace __gnu_debug } // namespace __gnu_debug +#if __cplusplus >= 201103L +namespace std _GLIBCXX_VISIBILITY(default) +{ +_GLIBCXX_BEGIN_NAMESPACE_VERSION + template<typename _Iterator, typename _Sequence, typename _Category> + struct pointer_traits<__gnu_debug::_Safe_iterator<_Iterator, _Sequence, + _Category>> + { + private: + using _Base = pointer_traits<_Iterator>; + + template<typename _Tp> + using __base_rebind = typename _Base::template rebind<_Tp>; + + public: + using element_type = typename _Base::element_type; + using pointer = + __gnu_debug::_Safe_iterator<_Iterator, _Sequence, _Category>; + using difference_type = typename _Base::difference_type; + + template<typename _Tp> + using rebind = + __gnu_debug::_Safe_iterator<__base_rebind<_Tp>, _Sequence, _Category>; + + static pointer + pointer_to(element_type& __e) noexcept + { return pointer(_Base::pointer_to(__e), nullptr); } + +#if __cplusplus >= 202002L + static element_type* + to_address(pointer __p) noexcept + { return __p.base(); } +#endif + }; +_GLIBCXX_END_NAMESPACE_VERSION +} // namespace +#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/20_util/pointer_traits/rebind.cc b/libstdc++-v3/testsuite/20_util/pointer_traits/rebind.cc index 159ea8f5294..b78e974d777 100644 --- a/libstdc++-v3/testsuite/20_util/pointer_traits/rebind.cc +++ b/libstdc++-v3/testsuite/20_util/pointer_traits/rebind.cc @@ -18,6 +18,7 @@ // { dg-do compile { target c++11 } } #include <memory> +#include <vector> using std::is_same; @@ -66,3 +67,13 @@ template<typename T, int = 0> }; // PR libstdc++/72793 specialization of pointer_traits is still well-formed: std::pointer_traits<CannotRebind<int>>::element_type e; + +static_assert(is_same<typename std::pointer_traits< + Rebind<typename std::vector<int>::iterator, long>>::element_type, + long>::value, + "iterator rebind"); + +static_assert(is_same<typename std::pointer_traits< + Rebind<typename std::vector<int>::const_iterator, long>>::element_type, + long>::value, + "const_iterator rebind"); 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..433c803beb1 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,9 @@ // { 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()); +int* r = std::__to_address(std::vector<int>(1, 1).begin()); +const int* s = std::__to_address(std::vector<int>(1, 1).cbegin());
On Thu, 30 Sept 2021 at 21:27, François Dumont via Libstdc++ <libstdc++@gcc.gnu.org> wrote: > > Here is the _Safe_iterator one. > > Doing so I noticed that pointer_traits rebind for __normal_iterator was > wrong and added tests on it. Oops, thanks! > For _Safe_iterator maybe I should specialize only when instantiated with > __normal_iterator ? Or maybe limit to random_access_iterator_tag ? Yes, I think that's a good idea, maybe only for __normal_iterator<T*, C>. I think that should cover all the cases we actually need to support. And could you change the existing pointer_traits<__normal_iterator<I, C>> partial specialization to only be valid for __normal_iterator<T*,C> as well? i.e. only for __normal_iterator that wraps a pointer (which is true for string::iterator, vector::iterator etc.) > Whatever the pointer_to implementation is problematic, we can only > produce singular iterator as I did ifor now. Hmm, yes. I think that's OK, because I don't think anybody is actually going to use that function. What matters is that it works with std::__to_address for C++17 mode, and that doesn't depend on pointer_to, so it's OK if it doesn't work usefully. Thanks!
On 02/10/21 12:29 am, Jonathan Wakely wrote: > On Thu, 30 Sept 2021 at 21:27, François Dumont via Libstdc++ > <libstdc++@gcc.gnu.org> wrote: >> Here is the _Safe_iterator one. >> >> Doing so I noticed that pointer_traits rebind for __normal_iterator was >> wrong and added tests on it. > Oops, thanks! > >> For _Safe_iterator maybe I should specialize only when instantiated with >> __normal_iterator ? Or maybe limit to random_access_iterator_tag ? > Yes, I think that's a good idea, maybe only for __normal_iterator<T*, > C>. I think that should cover all the cases we actually need to > support. > > And could you change the existing pointer_traits<__normal_iterator<I, > C>> partial specialization to only be valid for > __normal_iterator<T*,C> as well? i.e. only for __normal_iterator that > wraps a pointer (which is true for string::iterator, vector::iterator > etc.) Is the attached patch what you had in mind ? At least it is passing tests. > >> Whatever the pointer_to implementation is problematic, we can only >> produce singular iterator as I did ifor now. > Hmm, yes. I think that's OK, because I don't think anybody is actually > going to use that function. What matters is that it works with > std::__to_address for C++17 mode, and that doesn't depend on > pointer_to, so it's OK if it doesn't work usefully. > This is why I think that my patch of the unordered containers to support custom pointers is not so bad because it avoids usage of pointer_traits::to_pointer :-) diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h index 8afd6756613..f6dbe562505 100644 --- a/libstdc++-v3/include/bits/stl_iterator.h +++ b/libstdc++-v3/include/bits/stl_iterator.h @@ -1314,23 +1314,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // Need to specialize pointer_traits because the 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>> + template<typename _Tp, typename _Container> + struct pointer_traits<__gnu_cxx::__normal_iterator<_Tp*, _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; + using element_type = _Tp; + using pointer = __gnu_cxx::__normal_iterator<_Tp*, _Container>; + using difference_type = ptrdiff_t; - template<typename _Tp> - using rebind = __gnu_cxx::__normal_iterator<_Tp, _Container>; + template<typename _Up> + using rebind = __gnu_cxx::__normal_iterator<_Up*, _Container>; - static pointer - pointer_to(element_type& __e) noexcept - { return pointer(_Base::pointer_to(__e)); } + static _GLIBCXX20_CONSTEXPR pointer + pointer_to(__make_not_void<element_type>& __r) noexcept + { return pointer(std::addressof(__r)); } #if __cplusplus >= 202002L static element_type* diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h index 5584d06de5a..d2469574240 100644 --- a/libstdc++-v3/include/debug/safe_iterator.h +++ b/libstdc++-v3/include/debug/safe_iterator.h @@ -1013,6 +1013,45 @@ namespace __gnu_debug } // namespace __gnu_debug +#if __cplusplus >= 201103L +namespace std _GLIBCXX_VISIBILITY(default) +{ +_GLIBCXX_BEGIN_NAMESPACE_VERSION + template<typename _Tp, typename _Container, typename _Sequence> + struct pointer_traits<__gnu_debug::_Safe_iterator< + __gnu_cxx::__normal_iterator<_Tp*, _Container>, _Sequence, + std::random_access_iterator_tag>> + { + private: + using __base_ptr = + __gnu_cxx::__normal_iterator<_Tp*, _Container>; + + public: + using element_type = _Tp; + using pointer = __gnu_debug::_Safe_iterator< + __gnu_cxx::__normal_iterator<_Tp*, _Container>, _Sequence, + std::random_access_iterator_tag>; + using difference_type = ptrdiff_t; + + template<typename _Up> + using rebind = __gnu_debug::_Safe_iterator< + __gnu_cxx::__normal_iterator<_Up*, _Container>, _Sequence, + std::random_access_iterator_tag>; + + static pointer + pointer_to(element_type& __e) noexcept + { return { __base_ptr(std::addressof(__e)), nullptr }; } + +#if __cplusplus >= 202002L + static element_type* + to_address(const pointer& __p) noexcept + { return __p.base().base(); } +#endif + }; +_GLIBCXX_END_NAMESPACE_VERSION +} // namespace +#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/20_util/pointer_traits/rebind.cc b/libstdc++-v3/testsuite/20_util/pointer_traits/rebind.cc index 159ea8f5294..b78e974d777 100644 --- a/libstdc++-v3/testsuite/20_util/pointer_traits/rebind.cc +++ b/libstdc++-v3/testsuite/20_util/pointer_traits/rebind.cc @@ -18,6 +18,7 @@ // { dg-do compile { target c++11 } } #include <memory> +#include <vector> using std::is_same; @@ -66,3 +67,13 @@ template<typename T, int = 0> }; // PR libstdc++/72793 specialization of pointer_traits is still well-formed: std::pointer_traits<CannotRebind<int>>::element_type e; + +static_assert(is_same<typename std::pointer_traits< + Rebind<typename std::vector<int>::iterator, long>>::element_type, + long>::value, + "iterator rebind"); + +static_assert(is_same<typename std::pointer_traits< + Rebind<typename std::vector<int>::const_iterator, long>>::element_type, + long>::value, + "const_iterator rebind"); 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..433c803beb1 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,9 @@ // { 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()); +int* r = std::__to_address(std::vector<int>(1, 1).begin()); +const int* s = std::__to_address(std::vector<int>(1, 1).cbegin());
I would like to propose this alternative approach.
In this patch I make __normal_iterator and random iterator
_Safe_iterator compatible for pointer_traits primary template.
Regarding pointer_traits I wonder if it shouldn't check for the
to_pointer method availability and use per default: return {
std::addressof(__e) }; otherwise. This way we wouldn't have to provide a
pointer_to method on __normal_iterator.
François
On 02/10/21 12:29 am, Jonathan Wakely wrote:
> On Thu, 30 Sept 2021 at 21:27, François Dumont via Libstdc++
> <libstdc++@gcc.gnu.org> wrote:
>> Here is the _Safe_iterator one.
>>
>> Doing so I noticed that pointer_traits rebind for __normal_iterator was
>> wrong and added tests on it.
> Oops, thanks!
>
>> For _Safe_iterator maybe I should specialize only when instantiated with
>> __normal_iterator ? Or maybe limit to random_access_iterator_tag ?
> Yes, I think that's a good idea, maybe only for __normal_iterator<T*,
> C>. I think that should cover all the cases we actually need to
> support.
>
> And could you change the existing pointer_traits<__normal_iterator<I,
> C>> partial specialization to only be valid for
> __normal_iterator<T*,C> as well? i.e. only for __normal_iterator that
> wraps a pointer (which is true for string::iterator, vector::iterator
> etc.)
>
>> Whatever the pointer_to implementation is problematic, we can only
>> produce singular iterator as I did ifor now.
> Hmm, yes. I think that's OK, because I don't think anybody is actually
> going to use that function. What matters is that it works with
> std::__to_address for C++17 mode, and that doesn't depend on
> pointer_to, so it's OK if it doesn't work usefully.
>
> Thanks!
>
diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
index 8afd6756613..594d4d5b65f 100644
--- a/libstdc++-v3/include/bits/stl_iterator.h
+++ b/libstdc++-v3/include/bits/stl_iterator.h
@@ -1017,6 +1017,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
template<typename _Iterator, typename _Container>
class __normal_iterator
{
+#if __cplusplus >= 201103L
+ using __ite_ptr_traits = std::pointer_traits<_Iterator>;
+
+ template<typename _Tp>
+ using __ite_rebind = typename __ite_ptr_traits::template rebind<_Tp>;
+#endif
+
protected:
_Iterator _M_current;
@@ -1036,6 +1043,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
typedef typename __traits_type::reference reference;
typedef typename __traits_type::pointer pointer;
+#if __cplusplus >= 201103L
+ using element_type = typename __ite_ptr_traits::element_type;
+
+ template<typename _Up>
+ using rebind = __normal_iterator<__ite_rebind<_Up>, _Container>;
+
+ static __normal_iterator
+ pointer_to(element_type& __e)
+ { return { __ite_ptr_traits::pointer_to(__e) }; }
+#endif
+
#if __cplusplus > 201703L && __cpp_lib_concepts
using iterator_concept = std::__detail::__iter_concept<_Iterator>;
#endif
@@ -1311,34 +1329,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
{ return __it.base(); }
#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.
- 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(); }
-#endif
- };
-
/**
* @addtogroup iterators
* @{
diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h
index 5584d06de5a..b175c9e4a31 100644
--- a/libstdc++-v3/include/debug/safe_iterator.h
+++ b/libstdc++-v3/include/debug/safe_iterator.h
@@ -653,6 +653,13 @@ namespace __gnu_debug
: public _Safe_iterator<_Iterator, _Sequence,
std::bidirectional_iterator_tag>
{
+#if __cplusplus >= 201103L
+ using __ite_ptr_traits = std::pointer_traits<_Iterator>;
+
+ template<typename _Tp>
+ using __ite_rebind = typename __ite_ptr_traits::template rebind<_Tp>;
+#endif
+
typedef _Safe_iterator<_Iterator, _Sequence,
std::bidirectional_iterator_tag> _Safe_base;
typedef typename _Safe_base::_OtherIterator _OtherIterator;
@@ -672,6 +679,19 @@ namespace __gnu_debug
typedef typename _Safe_base::difference_type difference_type;
typedef typename _Safe_base::reference reference;
+
+#if __cplusplus >= 201103L
+ using element_type = typename __ite_ptr_traits::element_type;
+
+ template<typename _Up>
+ using rebind = _Safe_iterator<__ite_rebind<_Up>, _Sequence,
+ std::random_access_iterator_tag>;
+
+ static _Safe_iterator
+ pointer_to(element_type& __e)
+ { return { __ite_ptr_traits::pointer_to(__e), nullptr }; }
+#endif
+
/// @post the iterator is singular and unattached
_Safe_iterator() _GLIBCXX_NOEXCEPT { }
diff --git a/libstdc++-v3/testsuite/20_util/pointer_traits/rebind.cc b/libstdc++-v3/testsuite/20_util/pointer_traits/rebind.cc
index 159ea8f5294..b78e974d777 100644
--- a/libstdc++-v3/testsuite/20_util/pointer_traits/rebind.cc
+++ b/libstdc++-v3/testsuite/20_util/pointer_traits/rebind.cc
@@ -18,6 +18,7 @@
// { dg-do compile { target c++11 } }
#include <memory>
+#include <vector>
using std::is_same;
@@ -66,3 +67,13 @@ template<typename T, int = 0>
};
// PR libstdc++/72793 specialization of pointer_traits is still well-formed:
std::pointer_traits<CannotRebind<int>>::element_type e;
+
+static_assert(is_same<typename std::pointer_traits<
+ Rebind<typename std::vector<int>::iterator, long>>::element_type,
+ long>::value,
+ "iterator rebind");
+
+static_assert(is_same<typename std::pointer_traits<
+ Rebind<typename std::vector<int>::const_iterator, long>>::element_type,
+ long>::value,
+ "const_iterator rebind");
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..433c803beb1 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,9 @@
// { 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());
+int* r = std::__to_address(std::vector<int>(1, 1).begin());
+const int* s = std::__to_address(std::vector<int>(1, 1).cbegin());
On Sat, 2 Oct 2021 at 18:27, François Dumont wrote: > > I would like to propose this alternative approach. > > In this patch I make __normal_iterator and random iterator > _Safe_iterator compatible for pointer_traits primary template. > > Regarding pointer_traits I wonder if it shouldn't check for the > to_pointer method availability and use per default: return { > std::addressof(__e) }; otherwise. This way we wouldn't have to provide a > pointer_to method on __normal_iterator. But I would rather not have these members present in vector::iterator and string::iterator, in case users accidentally start to rely on them being present. Another option would be to overload std::__to_address so it knows how to get the address from __normal_iterator and _Safe_iterator.
On Sat, 2 Oct 2021 at 14:08, François Dumont <frs.dumont@gmail.com> wrote: > > On 02/10/21 12:29 am, Jonathan Wakely wrote: > > On Thu, 30 Sept 2021 at 21:27, François Dumont via Libstdc++ > > <libstdc++@gcc.gnu.org> wrote: > >> Here is the _Safe_iterator one. > >> > >> Doing so I noticed that pointer_traits rebind for __normal_iterator was > >> wrong and added tests on it. > > Oops, thanks! > > > >> For _Safe_iterator maybe I should specialize only when instantiated with > >> __normal_iterator ? Or maybe limit to random_access_iterator_tag ? > > Yes, I think that's a good idea, maybe only for __normal_iterator<T*, > > C>. I think that should cover all the cases we actually need to > > support. > > > > And could you change the existing pointer_traits<__normal_iterator<I, > > C>> partial specialization to only be valid for > > __normal_iterator<T*,C> as well? i.e. only for __normal_iterator that > > wraps a pointer (which is true for string::iterator, vector::iterator > > etc.) > > Is the attached patch what you had in mind ? Yes, although I don't think we need to us __make_not_void for to_pointer, because I don't think we would ever have __normal_iterator<void*, C>. I don't think it will even compile, because void* is not an iterator type, and iterator_traits<void*> is invalid (in C++20 it's constrained with is_object_v<T> and before C++20 it will just give an error, because void& is ill-formed).
On 02/10/21 10:24 pm, Jonathan Wakely wrote: > On Sat, 2 Oct 2021 at 18:27, François Dumont wrote: >> I would like to propose this alternative approach. >> >> In this patch I make __normal_iterator and random iterator >> _Safe_iterator compatible for pointer_traits primary template. >> >> Regarding pointer_traits I wonder if it shouldn't check for the >> to_pointer method availability and use per default: return { >> std::addressof(__e) }; otherwise. This way we wouldn't have to provide a >> pointer_to method on __normal_iterator. > But I would rather not have these members present in vector::iterator > and string::iterator, in case users accidentally start to rely on them > being present. Making pointer_traits friends would help but I do not like it neither. > > Another option would be to overload std::__to_address so it knows how > to get the address from __normal_iterator and _Safe_iterator. > > . I start thinking that rather than proposing not-useful and even incorrect code in the case of the _Safe_iterator<> it might be a better approach. Even the rebind for __normal_iterator is a little strange because when doing rebind on std::vector<int>::iterator for long it produces __normal_iterator<long*, std::vector<int>>, quite inconsistent even if useless. But there's something that I'm missing, what is the relation between __addressof and std::pointer_traits ? Is it that __builtin_addressof is using it ?
On 04/10/21 10:05 pm, François Dumont wrote: > On 02/10/21 10:24 pm, Jonathan Wakely wrote: >> On Sat, 2 Oct 2021 at 18:27, François Dumont wrote: >>> I would like to propose this alternative approach. >>> >>> In this patch I make __normal_iterator and random iterator >>> _Safe_iterator compatible for pointer_traits primary template. >>> >>> Regarding pointer_traits I wonder if it shouldn't check for the >>> to_pointer method availability and use per default: return { >>> std::addressof(__e) }; otherwise. This way we wouldn't have to >>> provide a >>> pointer_to method on __normal_iterator. >> But I would rather not have these members present in vector::iterator >> and string::iterator, in case users accidentally start to rely on them >> being present. > > Making pointer_traits friends would help but I do not like it neither. > > >> >> Another option would be to overload std::__to_address so it knows how >> to get the address from __normal_iterator and _Safe_iterator. >> >> . > > I start thinking that rather than proposing not-useful and even > incorrect code in the case of the _Safe_iterator<> it might be a > better approach. > > Even the rebind for __normal_iterator is a little strange because when > doing rebind on std::vector<int>::iterator for long it produces > __normal_iterator<long*, std::vector<int>>, quite inconsistent even if > useless. > > But there's something that I'm missing, what is the relation between > __addressof and std::pointer_traits ? Is it that __builtin_addressof > is using it ? > Ignore this last question, I realized that we are talking about __to_address, not __addressof.
On Mon, 4 Oct 2021 at 21:28, François Dumont via Libstdc++ <libstdc++@gcc.gnu.org> wrote: > > On 04/10/21 10:05 pm, François Dumont wrote: > > On 02/10/21 10:24 pm, Jonathan Wakely wrote: > >> On Sat, 2 Oct 2021 at 18:27, François Dumont wrote: > >>> I would like to propose this alternative approach. > >>> > >>> In this patch I make __normal_iterator and random iterator > >>> _Safe_iterator compatible for pointer_traits primary template. > >>> > >>> Regarding pointer_traits I wonder if it shouldn't check for the > >>> to_pointer method availability and use per default: return { > >>> std::addressof(__e) }; otherwise. This way we wouldn't have to > >>> provide a > >>> pointer_to method on __normal_iterator. > >> But I would rather not have these members present in vector::iterator > >> and string::iterator, in case users accidentally start to rely on them > >> being present. > > > > Making pointer_traits friends would help but I do not like it neither. > > > > > >> > >> Another option would be to overload std::__to_address so it knows how > >> to get the address from __normal_iterator and _Safe_iterator. > >> > >> . > > > > I start thinking that rather than proposing not-useful and even > > incorrect code in the case of the _Safe_iterator<> it might be a > > better approach. > > > > Even the rebind for __normal_iterator is a little strange because when > > doing rebind on std::vector<int>::iterator for long it produces > > __normal_iterator<long*, std::vector<int>>, quite inconsistent even if > > useless. > > > > But there's something that I'm missing, what is the relation between > > __addressof and std::pointer_traits ? Is it that __builtin_addressof > > is using it ? > > > Ignore this last question, I realized that we are talking about > __to_address, not __addressof. Yes, see the definition of std::__to_address in <bits/ptr_traits.h> and also PR 96416.
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 ? 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. 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. * include/debug/safe_iterator.h (std::__to_address(const __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<>, _Sequence>&)): New. * 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 Tested under Linux x86_64. Ok to commit ? François On 04/10/21 10:30 pm, Jonathan Wakely wrote: > On Mon, 4 Oct 2021 at 21:28, François Dumont via Libstdc++ > <libstdc++@gcc.gnu.org> wrote: >> On 04/10/21 10:05 pm, François Dumont wrote: >>> On 02/10/21 10:24 pm, Jonathan Wakely wrote: >>>> On Sat, 2 Oct 2021 at 18:27, François Dumont wrote: >>>>> I would like to propose this alternative approach. >>>>> >>>>> In this patch I make __normal_iterator and random iterator >>>>> _Safe_iterator compatible for pointer_traits primary template. >>>>> >>>>> Regarding pointer_traits I wonder if it shouldn't check for the >>>>> to_pointer method availability and use per default: return { >>>>> std::addressof(__e) }; otherwise. This way we wouldn't have to >>>>> provide a >>>>> pointer_to method on __normal_iterator. >>>> But I would rather not have these members present in vector::iterator >>>> and string::iterator, in case users accidentally start to rely on them >>>> being present. >>> Making pointer_traits friends would help but I do not like it neither. >>> >>> >>>> Another option would be to overload std::__to_address so it knows how >>>> to get the address from __normal_iterator and _Safe_iterator. >>>> >>>> . >>> I start thinking that rather than proposing not-useful and even >>> incorrect code in the case of the _Safe_iterator<> it might be a >>> better approach. >>> >>> Even the rebind for __normal_iterator is a little strange because when >>> doing rebind on std::vector<int>::iterator for long it produces >>> __normal_iterator<long*, std::vector<int>>, quite inconsistent even if >>> useless. >>> >>>
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 ?
Or perhaps just the _Safe_iterator_base branch in 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 ?
>
> 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.
>
> 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.
> * include/debug/safe_iterator.h
> (std::__to_address(const
> __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<>,
> _Sequence>&)):
> New.
> * 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
>
> Tested under Linux x86_64.
>
> Ok to commit ?
>
> François
>
>
> On 04/10/21 10:30 pm, Jonathan Wakely wrote:
>> On Mon, 4 Oct 2021 at 21:28, François Dumont via Libstdc++
>> <libstdc++@gcc.gnu.org> wrote:
>>> On 04/10/21 10:05 pm, François Dumont wrote:
>>>> On 02/10/21 10:24 pm, Jonathan Wakely wrote:
>>>>> On Sat, 2 Oct 2021 at 18:27, François Dumont wrote:
>>>>>> I would like to propose this alternative approach.
>>>>>>
>>>>>> In this patch I make __normal_iterator and random iterator
>>>>>> _Safe_iterator compatible for pointer_traits primary template.
>>>>>>
>>>>>> Regarding pointer_traits I wonder if it shouldn't check for the
>>>>>> to_pointer method availability and use per default: return {
>>>>>> std::addressof(__e) }; otherwise. This way we wouldn't have to
>>>>>> provide a
>>>>>> pointer_to method on __normal_iterator.
>>>>> But I would rather not have these members present in vector::iterator
>>>>> and string::iterator, in case users accidentally start to rely on
>>>>> them
>>>>> being present.
>>>> Making pointer_traits friends would help but I do not like it neither.
>>>>
>>>>
>>>>> Another option would be to overload std::__to_address so it knows how
>>>>> to get the address from __normal_iterator and _Safe_iterator.
>>>>>
>>>>> .
>>>> I start thinking that rather than proposing not-useful and even
>>>> incorrect code in the case of the _Safe_iterator<> it might be a
>>>> better approach.
>>>>
>>>> Even the rebind for __normal_iterator is a little strange because when
>>>> doing rebind on std::vector<int>::iterator for long it produces
>>>> __normal_iterator<long*, std::vector<int>>, quite inconsistent even if
>>>> useless.
>>>>
>>>>
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 ? > > Or perhaps just the _Safe_iterator_base branch in 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 ? >> >> 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. >> >> 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. >> * include/debug/safe_iterator.h >> (std::__to_address(const >> __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<>, >> _Sequence>&)): >> New. >> * 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 >> >> Tested under Linux x86_64. >> >> Ok to commit ? >> >> François >> >> >> On 04/10/21 10:30 pm, Jonathan Wakely wrote: >>> On Mon, 4 Oct 2021 at 21:28, François Dumont via Libstdc++ >>> <libstdc++@gcc.gnu.org> wrote: >>>> On 04/10/21 10:05 pm, François Dumont wrote: >>>>> On 02/10/21 10:24 pm, Jonathan Wakely wrote: >>>>>> On Sat, 2 Oct 2021 at 18:27, François Dumont wrote: >>>>>>> I would like to propose this alternative approach. >>>>>>> >>>>>>> In this patch I make __normal_iterator and random iterator >>>>>>> _Safe_iterator compatible for pointer_traits primary template. >>>>>>> >>>>>>> Regarding pointer_traits I wonder if it shouldn't check for the >>>>>>> to_pointer method availability and use per default: return { >>>>>>> std::addressof(__e) }; otherwise. This way we wouldn't have to >>>>>>> provide a >>>>>>> pointer_to method on __normal_iterator. >>>>>> But I would rather not have these members present in >>>>>> vector::iterator >>>>>> and string::iterator, in case users accidentally start to rely on >>>>>> them >>>>>> being present. >>>>> Making pointer_traits friends would help but I do not like it >>>>> neither. >>>>> >>>>> >>>>>> Another option would be to overload std::__to_address so it knows >>>>>> how >>>>>> to get the address from __normal_iterator and _Safe_iterator. >>>>>> >>>>>> . >>>>> I start thinking that rather than proposing not-useful and even >>>>> incorrect code in the case of the _Safe_iterator<> it might be a >>>>> better approach. >>>>> >>>>> Even the rebind for __normal_iterator is a little strange because >>>>> when >>>>> doing rebind on std::vector<int>::iterator for long it produces >>>>> __normal_iterator<long*, std::vector<int>>, quite inconsistent >>>>> even if >>>>> useless. >>>>> >>>>> >
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.
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.
On Wed, 15 Dec 2021 at 21:16, François Dumont <frs.dumont@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. > > > > >
diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h index c5b02408c1c..004d767224d 100644 --- a/libstdc++-v3/include/bits/stl_iterator.h +++ b/libstdc++-v3/include/bits/stl_iterator.h @@ -1285,6 +1285,34 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { return __it.base(); } #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. + 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(); } +#endif + }; + /** * @addtogroup iterators * @{ diff --git a/libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc b/libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc new file mode 100644 index 00000000000..510d627435f --- /dev/null +++ b/libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc @@ -0,0 +1,6 @@ +// { dg-do compile { target { c++11 } } } +#include <string> +#include <memory> + +char* p = std::__to_address(std::string("1").begin()); +const char* q = std::__to_address(std::string("2").cbegin());