[committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546]

[committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546]

[Jonathan Wakely]

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

This reduces the preprocessed size of <deque>, <string> and <vector> by
not including <bits/stl_algo.h> for std::remove and std::remove_if.

Also unwrap iterators using __niter_base, to avoid redundant debug mode
checks.

	PR libstdc++/92546
	* include/bits/erase_if.h (__erase_nodes_if): Use __niter_base to
	unwrap debug iterators.
	* include/bits/refwrap.h: Do not error if included in C++03.
	* include/bits/stl_algo.h (__remove_if): Move to ...
	* include/bits/stl_algobase.h (__remove_if): ... here.
	* include/std/deque (erase, erase_if): Use __remove_if instead of
	remove and remove_if.
	* include/std/string (erase, erase_if): Likewise.
	* include/std/vector (erase, erase_if): Likewise.

Tested powerpc64le-linux. Committed to trunk.

ff7793bea46
34e9407b3b4
b7e8fb5e482
6ccffeb56b9
e79bde6ada4
e5c093e515c
20751fad19e
9b790acc220
e3869a48fc2
44967af830a
dc1b29508d7
59ffa3e3dba
d71476c9df9
a09bb4a852f
cfb582f6279
c46ecb0112e
fb4d55ef61c
10b6d89badd
ce709ad3dc0
d335d73889d
681707ec28d
741c7350c08

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 9857 bytes --]

commit acf3a21cbc26b39b73c0006300f35ff017ddd6cb
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Oct 1 20:37:02 2021

    libstdc++: Reduce header dependencies for C++20 std::erase [PR92546]
    
    This reduces the preprocessed size of <deque>, <string> and <vector> by
    not including <bits/stl_algo.h> for std::remove and std::remove_if.
    
    Also unwrap iterators using __niter_base, to avoid redundant debug mode
    checks.
    
            PR libstdc++/92546
            * include/bits/erase_if.h (__erase_nodes_if): Use __niter_base to
            unwrap debug iterators.
            * include/bits/refwrap.h: Do not error if included in C++03.
            * include/bits/stl_algo.h (__remove_if): Move to ...
            * include/bits/stl_algobase.h (__remove_if): ... here.
            * include/std/deque (erase, erase_if): Use __remove_if instead of
            remove and remove_if.
            * include/std/string (erase, erase_if): Likewise.
            * include/std/vector (erase, erase_if): Likewise.

diff --git a/libstdc++-v3/include/bits/erase_if.h b/libstdc++-v3/include/bits/erase_if.h
index 8d1d23168fa..7716e1a953c 100644
--- a/libstdc++-v3/include/bits/erase_if.h
+++ b/libstdc++-v3/include/bits/erase_if.h
@@ -51,7 +51,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       __erase_nodes_if(_Container& __cont, _Predicate __pred)
       {
 	typename _Container::size_type __num = 0;
-	for (auto __iter = __cont.begin(), __last = __cont.end();
+	for (auto __iter = std::__niter_base(__cont.begin()),
+	     __last = std::__niter_base(__cont.end());
 	     __iter != __last;)
 	  {
 	    if (__pred(*__iter))
diff --git a/libstdc++-v3/include/bits/refwrap.h b/libstdc++-v3/include/bits/refwrap.h
index adfbe214693..a549efbce9a 100644
--- a/libstdc++-v3/include/bits/refwrap.h
+++ b/libstdc++-v3/include/bits/refwrap.h
@@ -32,9 +32,7 @@
 
 #pragma GCC system_header
 
-#if __cplusplus < 201103L
-# include <bits/c++0x_warning.h>
-#else
+#if __cplusplus >= 201103L
 
 #include <bits/move.h>
 #include <bits/invoke.h>
diff --git a/libstdc++-v3/include/bits/stl_algo.h b/libstdc++-v3/include/bits/stl_algo.h
index 90f3162ff90..bc611a95ef4 100644
--- a/libstdc++-v3/include/bits/stl_algo.h
+++ b/libstdc++-v3/include/bits/stl_algo.h
@@ -810,26 +810,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     }
 #endif // C++11
 
-  template<typename _ForwardIterator, typename _Predicate>
-    _GLIBCXX20_CONSTEXPR
-    _ForwardIterator
-    __remove_if(_ForwardIterator __first, _ForwardIterator __last,
-		_Predicate __pred)
-    {
-      __first = std::__find_if(__first, __last, __pred);
-      if (__first == __last)
-	return __first;
-      _ForwardIterator __result = __first;
-      ++__first;
-      for (; __first != __last; ++__first)
-	if (!__pred(__first))
-	  {
-	    *__result = _GLIBCXX_MOVE(*__first);
-	    ++__result;
-	  }
-      return __result;
-    }
-
   /**
    *  @brief Remove elements from a sequence.
    *  @ingroup mutating_algorithms
diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h
index 8627d59b589..0e0586836a6 100644
--- a/libstdc++-v3/include/bits/stl_algobase.h
+++ b/libstdc++-v3/include/bits/stl_algobase.h
@@ -2125,6 +2125,26 @@ _GLIBCXX_END_NAMESPACE_ALGO
       return __n;
     }
 
+  template<typename _ForwardIterator, typename _Predicate>
+    _GLIBCXX20_CONSTEXPR
+    _ForwardIterator
+    __remove_if(_ForwardIterator __first, _ForwardIterator __last,
+		_Predicate __pred)
+    {
+      __first = std::__find_if(__first, __last, __pred);
+      if (__first == __last)
+	return __first;
+      _ForwardIterator __result = __first;
+      ++__first;
+      for (; __first != __last; ++__first)
+	if (!__pred(__first))
+	  {
+	    *__result = _GLIBCXX_MOVE(*__first);
+	    ++__result;
+	  }
+      return __result;
+    }
+
 #if __cplusplus >= 201103L
   template<typename _ForwardIterator1, typename _ForwardIterator2,
 	   typename _BinaryPredicate>
diff --git a/libstdc++-v3/include/std/deque b/libstdc++-v3/include/std/deque
index c9a82110ad7..b2a7cee483a 100644
--- a/libstdc++-v3/include/std/deque
+++ b/libstdc++-v3/include/std/deque
@@ -58,13 +58,11 @@
 #pragma GCC system_header
 
 #include <bits/stl_algobase.h>
-#if __cplusplus > 201703L
-#  include <bits/stl_algo.h> // For remove and remove_if
-#endif // C++20
 #include <bits/allocator.h>
 #include <bits/stl_construct.h>
 #include <bits/stl_uninitialized.h>
 #include <bits/stl_deque.h>
+#include <bits/refwrap.h>
 #include <bits/range_access.h>
 #include <bits/deque.tcc>
 
@@ -97,9 +95,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline typename deque<_Tp, _Alloc>::size_type
     erase_if(deque<_Tp, _Alloc>& __cont, _Predicate __pred)
     {
+      using namespace __gnu_cxx;
       const auto __osz = __cont.size();
-      __cont.erase(std::remove_if(__cont.begin(), __cont.end(), __pred),
-		   __cont.end());
+      const auto __end = __cont.end();
+      auto __removed(std::__remove_if(std::__niter_base(__cont.begin()),
+				      std::__niter_base(__end),
+				      __ops::__pred_iter(std::ref(__pred))));
+      __cont.erase(std::__niter_wrap(__end, __removed), __end);
       return __osz - __cont.size();
     }
 
@@ -107,9 +109,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline typename deque<_Tp, _Alloc>::size_type
     erase(deque<_Tp, _Alloc>& __cont, const _Up& __value)
     {
+      using namespace __gnu_cxx;
       const auto __osz = __cont.size();
-      __cont.erase(std::remove(__cont.begin(), __cont.end(), __value),
-		   __cont.end());
+      const auto __end = __cont.end();
+      auto __removed(std::__remove_if(std::__niter_base(__cont.begin()),
+				      std::__niter_base(__end),
+				      __ops::__iter_equals_val(__value)));
+      __cont.erase(std::__niter_wrap(__end, __removed), __end);
       return __osz - __cont.size();
     }
 _GLIBCXX_END_NAMESPACE_VERSION
diff --git a/libstdc++-v3/include/std/string b/libstdc++-v3/include/std/string
index 0147e48d47a..7f994147a33 100644
--- a/libstdc++-v3/include/std/string
+++ b/libstdc++-v3/include/std/string
@@ -48,9 +48,7 @@
 #include <bits/stl_function.h> // For less
 #include <ext/numeric_traits.h>
 #include <bits/stl_algobase.h>
-#if __cplusplus > 201703L
-#  include <bits/stl_algo.h> // For remove and remove_if
-#endif // C++20
+#include <bits/refwrap.h>
 #include <bits/range_access.h>
 #include <bits/basic_string.h>
 #include <bits/basic_string.tcc>
@@ -125,9 +123,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline typename basic_string<_CharT, _Traits, _Alloc>::size_type
     erase_if(basic_string<_CharT, _Traits, _Alloc>& __cont, _Predicate __pred)
     {
+      using namespace __gnu_cxx;
+      using _It = typename basic_string<_CharT, _Traits, _Alloc>::iterator;
       const auto __osz = __cont.size();
-      __cont.erase(std::remove_if(__cont.begin(), __cont.end(), __pred),
-		   __cont.end());
+      _It __removed(std::__remove_if(std::__niter_base(__cont.begin()),
+				      std::__niter_base(__cont.end()),
+				      __ops::__pred_iter(std::ref(__pred))));
+      __cont.erase(__removed, __cont.end());
       return __osz - __cont.size();
     }
 
@@ -135,9 +137,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline typename basic_string<_CharT, _Traits, _Alloc>::size_type
     erase(basic_string<_CharT, _Traits, _Alloc>& __cont, const _Up& __value)
     {
+      using namespace __gnu_cxx;
+      using _It = typename basic_string<_CharT, _Traits, _Alloc>::iterator;
       const auto __osz = __cont.size();
-      __cont.erase(std::remove(__cont.begin(), __cont.end(), __value),
-		   __cont.end());
+      _It __removed(std::__remove_if(std::__niter_base(__cont.begin()),
+				      std::__niter_base(__cont.end()),
+				      __ops::__iter_equals_val(__value)));
+      __cont.erase(__removed, __cont.end());
       return __osz - __cont.size();
     }
 _GLIBCXX_END_NAMESPACE_VERSION
diff --git a/libstdc++-v3/include/std/vector b/libstdc++-v3/include/std/vector
index f804f4078ea..3b275e249fc 100644
--- a/libstdc++-v3/include/std/vector
+++ b/libstdc++-v3/include/std/vector
@@ -58,14 +58,12 @@
 #pragma GCC system_header
 
 #include <bits/stl_algobase.h>
-#if __cplusplus > 201703L
-#  include <bits/stl_algo.h> // For remove and remove_if
-#endif // C++20
 #include <bits/allocator.h>
 #include <bits/stl_construct.h>
 #include <bits/stl_uninitialized.h>
 #include <bits/stl_vector.h>
 #include <bits/stl_bvector.h>
+#include <bits/refwrap.h>
 #include <bits/range_access.h>
 
 #ifndef _GLIBCXX_EXPORT_TEMPLATE
@@ -107,9 +105,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline typename vector<_Tp, _Alloc>::size_type
     erase_if(vector<_Tp, _Alloc>& __cont, _Predicate __pred)
     {
+      using namespace __gnu_cxx;
       const auto __osz = __cont.size();
-      __cont.erase(std::remove_if(__cont.begin(), __cont.end(), __pred),
-		   __cont.end());
+      const auto __end = __cont.end();
+      auto __removed(std::__remove_if(std::__niter_base(__cont.begin()),
+				      std::__niter_base(__end),
+				      __ops::__pred_iter(std::ref(__pred))));
+      __cont.erase(std::__niter_wrap(__end, __removed), __end);
       return __osz - __cont.size();
     }
 
@@ -117,9 +119,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline typename vector<_Tp, _Alloc>::size_type
     erase(vector<_Tp, _Alloc>& __cont, const _Up& __value)
     {
+      using namespace __gnu_cxx;
       const auto __osz = __cont.size();
-      __cont.erase(std::remove(__cont.begin(), __cont.end(), __value),
-		   __cont.end());
+      const auto __end = __cont.end();
+      auto __removed(std::__remove_if(std::__niter_base(__cont.begin()),
+				      std::__niter_base(__end),
+				      __ops::__iter_equals_val(__value)));
+      __cont.erase(std::__niter_wrap(__end, __removed), __end);
       return __osz - __cont.size();
     }
 _GLIBCXX_END_NAMESPACE_VERSION

Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546]

[François Dumont]

On 01/10/21 9:38 pm, Jonathan Wakely via Libstdc++ wrote:
> This reduces the preprocessed size of <deque>, <string> and <vector> by
> not including <bits/stl_algo.h> for std::remove and std::remove_if.
>
> Also unwrap iterators using __niter_base, to avoid redundant debug mode
> checks.

I don't know if you noticed but the __niter_base is a no-op here.

__niter_base unwrap only random access iterators because it is the only 
category for which we know that we have been able to confirm validity or 
not.

We would need to introduce another function to do this or specialize in 
some way erase_if for debug containers. I'll try to find a solution.


>
> 	PR libstdc++/92546
> 	* include/bits/erase_if.h (__erase_nodes_if): Use __niter_base to
> 	unwrap debug iterators.
> 	* include/bits/refwrap.h: Do not error if included in C++03.
> 	* include/bits/stl_algo.h (__remove_if): Move to ...
> 	* include/bits/stl_algobase.h (__remove_if): ... here.
> 	* include/std/deque (erase, erase_if): Use __remove_if instead of
> 	remove and remove_if.
> 	* include/std/string (erase, erase_if): Likewise.
> 	* include/std/vector (erase, erase_if): Likewise.
>
> Tested powerpc64le-linux. Committed to trunk.
>
> ff7793bea46
> 34e9407b3b4
> b7e8fb5e482
> 6ccffeb56b9
> e79bde6ada4
> e5c093e515c
> 20751fad19e
> 9b790acc220
> e3869a48fc2
> 44967af830a
> dc1b29508d7
> 59ffa3e3dba
> d71476c9df9
> a09bb4a852f
> cfb582f6279
> c46ecb0112e
> fb4d55ef61c
> 10b6d89badd
> ce709ad3dc0
> d335d73889d
> 681707ec28d
> 741c7350c08

Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546]

[Jonathan Wakely]

On Sat, 2 Oct 2021, 13:02 François Dumont via Libstdc++, <
libstdc++@gcc.gnu.org> wrote:

> On 01/10/21 9:38 pm, Jonathan Wakely via Libstdc++ wrote:
> > This reduces the preprocessed size of <deque>, <string> and <vector> by
> > not including <bits/stl_algo.h> for std::remove and std::remove_if.
> >
> > Also unwrap iterators using __niter_base, to avoid redundant debug mode
> > checks.
>
> I don't know if you noticed but the __niter_base is a no-op here.
>

Oh, I didn't notice.


> __niter_base unwrap only random access iterators because it is the only
> category for which we know that we have been able to confirm validity or
> not.
>

But these are all random access. I must be missing something.


> We would need to introduce another function to do this or specialize in
> some way erase_if for debug containers. I'll try to find a solution.
>

OK, thanks. Maybe we can just leave the checking there. I wanted to avoid
the overhead because we know that the iterator range is valid. Any checks
done on each increment and equality comparison are wasteful, as they will
never fail.




>
> >
> >       PR libstdc++/92546
> >       * include/bits/erase_if.h (__erase_nodes_if): Use __niter_base to
> >       unwrap debug iterators.
> >       * include/bits/refwrap.h: Do not error if included in C++03.
> >       * include/bits/stl_algo.h (__remove_if): Move to ...
> >       * include/bits/stl_algobase.h (__remove_if): ... here.
> >       * include/std/deque (erase, erase_if): Use __remove_if instead of
> >       remove and remove_if.
> >       * include/std/string (erase, erase_if): Likewise.
> >       * include/std/vector (erase, erase_if): Likewise.
> >
> > Tested powerpc64le-linux. Committed to trunk.
> >
> > ff7793bea46
> > 34e9407b3b4
> > b7e8fb5e482
> > 6ccffeb56b9
> > e79bde6ada4
> > e5c093e515c
> > 20751fad19e
> > 9b790acc220
> > e3869a48fc2
> > 44967af830a
> > dc1b29508d7
> > 59ffa3e3dba
> > d71476c9df9
> > a09bb4a852f
> > cfb582f6279
> > c46ecb0112e
> > fb4d55ef61c
> > 10b6d89badd
> > ce709ad3dc0
> > d335d73889d
> > 681707ec28d
> > 741c7350c08
>
>
>

Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546]

[François Dumont]

On 02/10/21 2:47 pm, Jonathan Wakely wrote:
>
>
> On Sat, 2 Oct 2021, 13:02 François Dumont via Libstdc++, 
> <libstdc++@gcc.gnu.org <mailto:libstdc%2B%2B@gcc.gnu.org>> wrote:
>
>     On 01/10/21 9:38 pm, Jonathan Wakely via Libstdc++ wrote:
>     > This reduces the preprocessed size of <deque>, <string> and
>     <vector> by
>     > not including <bits/stl_algo.h> for std::remove and std::remove_if.
>     >
>     > Also unwrap iterators using __niter_base, to avoid redundant
>     debug mode
>     > checks.
>
>     I don't know if you noticed but the __niter_base is a no-op here.
>
>
> Oh, I didn't notice.
>
>
>     __niter_base unwrap only random access iterators because it is the
>     only
>     category for which we know that we have been able to confirm
>     validity or
>     not.
>
>
> But these are all random access. I must be missing something.

It is in a method called '__erases_node_if', I'm not aware of any 
node-based container providing random access iterators.

Moreover I just noticed that if it was really doing anything then you 
would be missing the std::__niter_wrap in the __cont.erase(__iter), it 
just wouldn't compile.

>
>     We would need to introduce another function to do this or
>     specialize in
>     some way erase_if for debug containers. I'll try to find a solution.
>
>
> OK, thanks. Maybe we can just leave the checking there. I wanted to 
> avoid the overhead because we know that the iterator range is valid. 
> Any checks done on each increment and equality comparison are 
> wasteful, as they will never fail.

Yes, that would be better indeed.

But doing it this way you still have the overhead of the _Safe_iterator 
addition to the list of the safe container iterators, so a mutex 
lock/unlock.

I'll try to find out how to get a normal iterator from a safe container 
even if in this case we will have to support operations on safe 
container with normal iterators.

Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546]

[Jonathan Wakely]

On Thu, 7 Oct 2021, 07:35 François Dumont, <frs.dumont@gmail.com> wrote:

> On 02/10/21 2:47 pm, Jonathan Wakely wrote:
>
>
>
> On Sat, 2 Oct 2021, 13:02 François Dumont via Libstdc++, <
> libstdc++@gcc.gnu.org> wrote:
>
>> On 01/10/21 9:38 pm, Jonathan Wakely via Libstdc++ wrote:
>> > This reduces the preprocessed size of <deque>, <string> and <vector> by
>> > not including <bits/stl_algo.h> for std::remove and std::remove_if.
>> >
>> > Also unwrap iterators using __niter_base, to avoid redundant debug mode
>> > checks.
>>
>> I don't know if you noticed but the __niter_base is a no-op here.
>>
>
> Oh, I didn't notice.
>
>
>> __niter_base unwrap only random access iterators because it is the only
>> category for which we know that we have been able to confirm validity or
>> not.
>>
>
> But these are all random access. I must be missing something.
>
> It is in a method called '__erases_node_if', I'm not aware of any
> node-based container providing random access iterators.
>

Ah, I thought you meant the deque, string and vector part of the patch,
sorry.

Moreover I just noticed that if it was really doing anything then you would
> be missing the std::__niter_wrap in the __cont.erase(__iter), it just
> wouldn't compile.
>
>
>> We would need to introduce another function to do this or specialize in
>> some way erase_if for debug containers. I'll try to find a solution.
>>
>
> OK, thanks. Maybe we can just leave the checking there. I wanted to avoid
> the overhead because we know that the iterator range is valid. Any checks
> done on each increment and equality comparison are wasteful, as they will
> never fail.
>
> Yes, that would be better indeed.
>
> But doing it this way you still have the overhead of the _Safe_iterator
> addition to the list of the safe container iterators, so a mutex
> lock/unlock.
>
> I'll try to find out how to get a normal iterator from a safe container
> even if in this case we will have to support operations on safe container
> with normal iterators.
>
When we know the type, we could use __cont._GLIBCXX_STD_C::vector<T,
Alloc>::begin() to access the base version directly. But for the node
containers we have a generic function where we don't know the exact type.
Is the _Base typedef accessible? __cont.decltype(__cont)::_Base::begin()
would work, but it's ugly.

Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546]

[Jonathan Wakely]

On Thu, 7 Oct 2021 at 07:52, Jonathan Wakely via Libstdc++
<libstdc++@gcc.gnu.org> wrote:
>
> On Thu, 7 Oct 2021, 07:35 François Dumont, <frs.dumont@gmail.com> wrote:
>
> > On 02/10/21 2:47 pm, Jonathan Wakely wrote:
> >
> >
> >
> > On Sat, 2 Oct 2021, 13:02 François Dumont via Libstdc++, <
> > libstdc++@gcc.gnu.org> wrote:
> >
> >> On 01/10/21 9:38 pm, Jonathan Wakely via Libstdc++ wrote:
> >> > This reduces the preprocessed size of <deque>, <string> and <vector> by
> >> > not including <bits/stl_algo.h> for std::remove and std::remove_if.
> >> >
> >> > Also unwrap iterators using __niter_base, to avoid redundant debug mode
> >> > checks.
> >>
> >> I don't know if you noticed but the __niter_base is a no-op here.
> >>
> >
> > Oh, I didn't notice.
> >
> >
> >> __niter_base unwrap only random access iterators because it is the only
> >> category for which we know that we have been able to confirm validity or
> >> not.
> >>
> >
> > But these are all random access. I must be missing something.
> >
> > It is in a method called '__erases_node_if', I'm not aware of any
> > node-based container providing random access iterators.
> >
>
> Ah, I thought you meant the deque, string and vector part of the patch,
> sorry.
>
> Moreover I just noticed that if it was really doing anything then you would
> > be missing the std::__niter_wrap in the __cont.erase(__iter), it just
> > wouldn't compile.
> >
> >
> >> We would need to introduce another function to do this or specialize in
> >> some way erase_if for debug containers. I'll try to find a solution.
> >>
> >
> > OK, thanks. Maybe we can just leave the checking there. I wanted to avoid
> > the overhead because we know that the iterator range is valid. Any checks
> > done on each increment and equality comparison are wasteful, as they will
> > never fail.
> >
> > Yes, that would be better indeed.
> >
> > But doing it this way you still have the overhead of the _Safe_iterator
> > addition to the list of the safe container iterators, so a mutex
> > lock/unlock.
> >
> > I'll try to find out how to get a normal iterator from a safe container
> > even if in this case we will have to support operations on safe container
> > with normal iterators.
> >
> When we know the type, we could use __cont._GLIBCXX_STD_C::vector<T,
> Alloc>::begin() to access the base version directly. But for the node
> containers we have a generic function where we don't know the exact type.
> Is the _Base typedef accessible? __cont.decltype(__cont)::_Base::begin()
> would work, but it's ugly.

The solution is simple:

    erase_if(set<_Key, _Compare, _Alloc>& __cont, _Predicate __pred)
-    { return __detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::set<_Key, _Compare, _Alloc>& __c = __cont;
+      return __detail::__erase_nodes_if(__c, __pred);
+    }


i.e. just bind a reference to the non-debug container type. For
non-debug mode, that is a no-op. For debug mode it binds to the base,
and the rest of the function works directly on the base, without safe
iterators.

I'm testing the patch now.

[committed] libstdc++: Avoid debug checks in uniform container erasure functions

[Jonathan Wakely]

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

On Thu, 7 Oct 2021 at 15:02, Jonathan Wakely wrote:
>
> The solution is simple:
>
>     erase_if(set<_Key, _Compare, _Alloc>& __cont, _Predicate __pred)
> -    { return __detail::__erase_nodes_if(__cont, __pred); }
> +    {
> +      _GLIBCXX_STD_C::set<_Key, _Compare, _Alloc>& __c = __cont;
> +      return __detail::__erase_nodes_if(__c, __pred);
> +    }
>
>
> i.e. just bind a reference to the non-debug container type. For
> non-debug mode, that is a no-op. For debug mode it binds to the base,
> and the rest of the function works directly on the base, without safe
> iterators.
>
> I'm testing the patch now.

Tested on powerpc64le-linux and pushed to trunk now.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 19612 bytes --]

commit 561078480ffb5adb68577276c6b23e4ee7b39272
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Oct 7 14:40:26 2021

    libstdc++: Avoid debug checks in uniform container erasure functions
    
    In commit r12-4083 I tried to make the std::erase and std::erase_if
    function avoid the unnecessary overhead of safe iterators. It didn't
    work, for two reasons. Firstly, for the RB tree containers the
    __niter_base function is a no-op (because the iterators aren't
    random access) so the safe iterators were still used. Secondly, for the
    cases where __niter_base did remove the safe iterator layer, there was
    still unnecessary overhead to create a new safe iterator and link it to
    the container.
    
    This solves the problem by simply binding a reference to the non-debug
    version of the conainer. For normal mode this is a no-op, and for debug
    mode it binds a reference to the debug container's base class. That
    means the rest of the function operates directly on the non-debug
    container, and avoids all checking.
    
    For std::basic_string there's no need to unwrap anything, because we use
    std::basic_string directly in debug mode anyway.
    
    libstdc++-v3/ChangeLog:
    
            * include/bits/erase_if.h (__erase_nodes_if): Remove redundant
            __niter_base calls.
            * include/std/string (erase, erase_if): Likewise.
            * include/std/deque (erase, erase_if): Access non-debug
            container directly.
            * include/std/map (erase, erase_if): Likewise.
            * include/std/set (erase, erase_if): Likewise.
            * include/std/unordered_map (erase, erase_if): Likewise.
            * include/std/unordered_set (erase, erase_if): Likewise.
            * include/std/vector (erase, erase_if): Likewise.
            * include/experimental/deque (erase, erase_if): Likewise.
            * include/experimental/map (erase, erase_if): Likewise.
            * include/experimental/set (erase, erase_if): Likewise.
            * include/experimental/unordered_map (erase, erase_if):
            Likewise.
            * include/experimental/unordered_set (erase, erase_if):
            Likewise.
            * include/experimental/vector (erase, erase_if): Likewise.

diff --git a/libstdc++-v3/include/bits/erase_if.h b/libstdc++-v3/include/bits/erase_if.h
index 7716e1a953c..8d1d23168fa 100644
--- a/libstdc++-v3/include/bits/erase_if.h
+++ b/libstdc++-v3/include/bits/erase_if.h
@@ -51,8 +51,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       __erase_nodes_if(_Container& __cont, _Predicate __pred)
       {
 	typename _Container::size_type __num = 0;
-	for (auto __iter = std::__niter_base(__cont.begin()),
-	     __last = std::__niter_base(__cont.end());
+	for (auto __iter = __cont.begin(), __last = __cont.end();
 	     __iter != __last;)
 	  {
 	    if (__pred(*__iter))
diff --git a/libstdc++-v3/include/experimental/deque b/libstdc++-v3/include/experimental/deque
index a76fb659bbf..710833ebcad 100644
--- a/libstdc++-v3/include/experimental/deque
+++ b/libstdc++-v3/include/experimental/deque
@@ -50,16 +50,16 @@ inline namespace fundamentals_v2
     inline void
     erase_if(deque<_Tp, _Alloc>& __cont, _Predicate __pred)
     {
-      __cont.erase(std::remove_if(__cont.begin(), __cont.end(), __pred),
-		   __cont.end());
+      _GLIBCXX_STD_C::deque<_Tp, _Alloc>& __c = __cont;
+      __c.erase(std::remove_if(__c.begin(), __c.end(), __pred), __c.end());
     }
 
   template<typename _Tp, typename _Alloc, typename _Up>
     inline void
     erase(deque<_Tp, _Alloc>& __cont, const _Up& __value)
     {
-      __cont.erase(std::remove(__cont.begin(), __cont.end(), __value),
-		   __cont.end());
+      _GLIBCXX_STD_C::deque<_Tp, _Alloc>& __c = __cont;
+      __c.erase(std::remove(__c.begin(), __c.end(), __value), __c.end());
     }
 
   namespace pmr {
diff --git a/libstdc++-v3/include/experimental/map b/libstdc++-v3/include/experimental/map
index 0c0f42222f5..ef69fadf944 100644
--- a/libstdc++-v3/include/experimental/map
+++ b/libstdc++-v3/include/experimental/map
@@ -50,13 +50,19 @@ inline namespace fundamentals_v2
 	   typename _Predicate>
     inline void
     erase_if(map<_Key, _Tp, _Compare, _Alloc>& __cont, _Predicate __pred)
-    { std::__detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::map<_Key, _Tp, _Compare, _Alloc>& __c = __cont;
+      std::__detail::__erase_nodes_if(__c, __pred);
+    }
 
   template<typename _Key, typename _Tp, typename _Compare, typename _Alloc,
 	   typename _Predicate>
     inline void
     erase_if(multimap<_Key, _Tp, _Compare, _Alloc>& __cont, _Predicate __pred)
-    { std::__detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::multimap<_Key, _Tp, _Compare, _Alloc>& __c = __cont;
+      std::__detail::__erase_nodes_if(__c, __pred);
+    }
 
   namespace pmr {
     template<typename _Key, typename _Tp, typename _Compare = less<_Key>>
diff --git a/libstdc++-v3/include/experimental/set b/libstdc++-v3/include/experimental/set
index c3f5433e995..7a5986aec0e 100644
--- a/libstdc++-v3/include/experimental/set
+++ b/libstdc++-v3/include/experimental/set
@@ -50,13 +50,19 @@ inline namespace fundamentals_v2
 	   typename _Predicate>
     inline void
     erase_if(set<_Key, _Compare, _Alloc>& __cont, _Predicate __pred)
-    { std::__detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::set<_Key, _Compare, _Alloc>& __c = __cont;
+      std::__detail::__erase_nodes_if(__c, __pred);
+    }
 
   template<typename _Key, typename _Compare, typename _Alloc,
 	   typename _Predicate>
     inline void
     erase_if(multiset<_Key, _Compare, _Alloc>& __cont, _Predicate __pred)
-    { std::__detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::multiset<_Key, _Compare, _Alloc>& __c = __cont;
+      std::__detail::__erase_nodes_if(__c, __pred);
+    }
 
   namespace pmr {
     template<typename _Key, typename _Compare = less<_Key>>
diff --git a/libstdc++-v3/include/experimental/unordered_map b/libstdc++-v3/include/experimental/unordered_map
index 0b915ab13e5..eba989713fa 100644
--- a/libstdc++-v3/include/experimental/unordered_map
+++ b/libstdc++-v3/include/experimental/unordered_map
@@ -51,14 +51,22 @@ inline namespace fundamentals_v2
     inline void
     erase_if(unordered_map<_Key, _Tp, _Hash, _CPred, _Alloc>& __cont,
 	     _Predicate __pred)
-    { std::__detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::unordered_map<_Key, _Tp, _Hash, _CPred, _Alloc>& __c
+	= __cont;
+      std::__detail::__erase_nodes_if(__c, __pred);
+    }
 
   template<typename _Key, typename _Tp, typename _Hash, typename _CPred,
 	   typename _Alloc, typename _Predicate>
     inline void
     erase_if(unordered_multimap<_Key, _Tp, _Hash, _CPred, _Alloc>& __cont,
 	     _Predicate __pred)
-    { std::__detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::unordered_multimap<_Key, _Tp, _Hash, _CPred, _Alloc>& __c
+	= __cont;
+      std::__detail::__erase_nodes_if(__c, __pred);
+    }
 
   namespace pmr {
     template<typename _Key, typename _Tp, typename _Hash = hash<_Key>,
diff --git a/libstdc++-v3/include/experimental/unordered_set b/libstdc++-v3/include/experimental/unordered_set
index 87db4464401..bc5cc11419e 100644
--- a/libstdc++-v3/include/experimental/unordered_set
+++ b/libstdc++-v3/include/experimental/unordered_set
@@ -51,14 +51,21 @@ inline namespace fundamentals_v2
     inline void
     erase_if(unordered_set<_Key, _Hash, _CPred, _Alloc>& __cont,
 	     _Predicate __pred)
-    { std::__detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::unordered_set<_Key, _Hash, _CPred, _Alloc>& __c = __cont;
+      std::__detail::__erase_nodes_if(__c, __pred);
+    }
 
   template<typename _Key, typename _Hash, typename _CPred, typename _Alloc,
 	   typename _Predicate>
     inline void
     erase_if(unordered_multiset<_Key, _Hash, _CPred, _Alloc>& __cont,
 	     _Predicate __pred)
-    { std::__detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::unordered_multiset<_Key, _Hash, _CPred, _Alloc>& __c
+	= __cont;
+      std::__detail::__erase_nodes_if(__c, __pred);
+    }
 
   namespace pmr {
     template<typename _Key, typename _Hash = hash<_Key>,
diff --git a/libstdc++-v3/include/experimental/vector b/libstdc++-v3/include/experimental/vector
index a14aedf3364..c45a500ef5e 100644
--- a/libstdc++-v3/include/experimental/vector
+++ b/libstdc++-v3/include/experimental/vector
@@ -52,16 +52,16 @@ inline namespace fundamentals_v2
     inline void
     erase_if(vector<_Tp, _Alloc>& __cont, _Predicate __pred)
     {
-      __cont.erase(std::remove_if(__cont.begin(), __cont.end(), __pred),
-		   __cont.end());
+      _GLIBCXX_STD_C::vector<_Tp, _Alloc>& __c = __cont;
+      __c.erase(std::remove_if(__c.begin(), __c.end(), __pred), __c.end());
     }
 
   template<typename _Tp, typename _Alloc, typename _Up>
     inline void
     erase(vector<_Tp, _Alloc>& __cont, const _Up& __value)
     {
-      __cont.erase(std::remove(__cont.begin(), __cont.end(), __value),
-		   __cont.end());
+      _GLIBCXX_STD_C::vector<_Tp, _Alloc>& __c = __cont;
+      __c.erase(std::remove(__c.begin(), __c.end(), __value), __c.end());
     }
 
   namespace pmr {
diff --git a/libstdc++-v3/include/std/deque b/libstdc++-v3/include/std/deque
index b2a7cee483a..71993e757a5 100644
--- a/libstdc++-v3/include/std/deque
+++ b/libstdc++-v3/include/std/deque
@@ -95,28 +95,28 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline typename deque<_Tp, _Alloc>::size_type
     erase_if(deque<_Tp, _Alloc>& __cont, _Predicate __pred)
     {
+      _GLIBCXX_STD_C::deque<_Tp, _Alloc>& __c = __cont;
       using namespace __gnu_cxx;
-      const auto __osz = __cont.size();
-      const auto __end = __cont.end();
-      auto __removed(std::__remove_if(std::__niter_base(__cont.begin()),
-				      std::__niter_base(__end),
-				      __ops::__pred_iter(std::ref(__pred))));
-      __cont.erase(std::__niter_wrap(__end, __removed), __end);
-      return __osz - __cont.size();
+      const auto __osz = __c.size();
+      const auto __end = __c.end();
+      auto __removed = std::__remove_if(__c.begin(), __end,
+					__ops::__pred_iter(std::ref(__pred)));
+      __c.erase(__removed, __end);
+      return __osz - __c.size();
     }
 
   template<typename _Tp, typename _Alloc, typename _Up>
     inline typename deque<_Tp, _Alloc>::size_type
     erase(deque<_Tp, _Alloc>& __cont, const _Up& __value)
     {
+      _GLIBCXX_STD_C::deque<_Tp, _Alloc>& __c = __cont;
       using namespace __gnu_cxx;
-      const auto __osz = __cont.size();
-      const auto __end = __cont.end();
-      auto __removed(std::__remove_if(std::__niter_base(__cont.begin()),
-				      std::__niter_base(__end),
-				      __ops::__iter_equals_val(__value)));
-      __cont.erase(std::__niter_wrap(__end, __removed), __end);
-      return __osz - __cont.size();
+      const auto __osz = __c.size();
+      const auto __end = __c.end();
+      auto __removed = std::__remove_if(__c.begin(), __end,
+					__ops::__iter_equals_val(__value));
+      __c.erase(__removed, __end);
+      return __osz - __c.size();
     }
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace std
diff --git a/libstdc++-v3/include/std/map b/libstdc++-v3/include/std/map
index 44bd44b5922..29265580995 100644
--- a/libstdc++-v3/include/std/map
+++ b/libstdc++-v3/include/std/map
@@ -95,13 +95,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Predicate>
     inline typename map<_Key, _Tp, _Compare, _Alloc>::size_type
     erase_if(map<_Key, _Tp, _Compare, _Alloc>& __cont, _Predicate __pred)
-    { return __detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::map<_Key, _Tp, _Compare, _Alloc>& __c = __cont;
+      return __detail::__erase_nodes_if(__c, __pred);
+    }
 
   template<typename _Key, typename _Tp, typename _Compare, typename _Alloc,
 	   typename _Predicate>
     inline typename multimap<_Key, _Tp, _Compare, _Alloc>::size_type
     erase_if(multimap<_Key, _Tp, _Compare, _Alloc>& __cont, _Predicate __pred)
-    { return __detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::multimap<_Key, _Tp, _Compare, _Alloc>& __c = __cont;
+      return __detail::__erase_nodes_if(__c, __pred);
+    }
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace std
 #endif // C++20
diff --git a/libstdc++-v3/include/std/set b/libstdc++-v3/include/std/set
index f1e1864937a..24e6e633624 100644
--- a/libstdc++-v3/include/std/set
+++ b/libstdc++-v3/include/std/set
@@ -91,13 +91,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	   typename _Predicate>
     inline typename set<_Key, _Compare, _Alloc>::size_type
     erase_if(set<_Key, _Compare, _Alloc>& __cont, _Predicate __pred)
-    { return __detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::set<_Key, _Compare, _Alloc>& __c = __cont;
+      return __detail::__erase_nodes_if(__c, __pred);
+    }
 
   template<typename _Key, typename _Compare, typename _Alloc,
 	   typename _Predicate>
     inline typename multiset<_Key, _Compare, _Alloc>::size_type
     erase_if(multiset<_Key, _Compare, _Alloc>& __cont, _Predicate __pred)
-    { return __detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::multiset<_Key, _Compare, _Alloc>& __c = __cont;
+      return __detail::__erase_nodes_if(__c, __pred);
+    }
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace std
 #endif // C++20
diff --git a/libstdc++-v3/include/std/string b/libstdc++-v3/include/std/string
index 7f994147a33..95412b6f7a3 100644
--- a/libstdc++-v3/include/std/string
+++ b/libstdc++-v3/include/std/string
@@ -124,12 +124,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     erase_if(basic_string<_CharT, _Traits, _Alloc>& __cont, _Predicate __pred)
     {
       using namespace __gnu_cxx;
-      using _It = typename basic_string<_CharT, _Traits, _Alloc>::iterator;
       const auto __osz = __cont.size();
-      _It __removed(std::__remove_if(std::__niter_base(__cont.begin()),
-				      std::__niter_base(__cont.end()),
-				      __ops::__pred_iter(std::ref(__pred))));
-      __cont.erase(__removed, __cont.end());
+      const auto __end = __cont.end();
+      auto __removed = std::__remove_if(__cont.begin(), __end,
+					__ops::__pred_iter(std::ref(__pred)));
+      __cont.erase(__removed, __end);
       return __osz - __cont.size();
     }
 
@@ -138,12 +137,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     erase(basic_string<_CharT, _Traits, _Alloc>& __cont, const _Up& __value)
     {
       using namespace __gnu_cxx;
-      using _It = typename basic_string<_CharT, _Traits, _Alloc>::iterator;
       const auto __osz = __cont.size();
-      _It __removed(std::__remove_if(std::__niter_base(__cont.begin()),
-				      std::__niter_base(__cont.end()),
-				      __ops::__iter_equals_val(__value)));
-      __cont.erase(__removed, __cont.end());
+      const auto __end = __cont.end();
+      auto __removed = std::__remove_if(__cont.begin(), __end,
+					__ops::__iter_equals_val(__value));
+      __cont.erase(__removed, __end);
       return __osz - __cont.size();
     }
 _GLIBCXX_END_NAMESPACE_VERSION
diff --git a/libstdc++-v3/include/std/unordered_map b/libstdc++-v3/include/std/unordered_map
index e6715069362..774c21fc28b 100644
--- a/libstdc++-v3/include/std/unordered_map
+++ b/libstdc++-v3/include/std/unordered_map
@@ -83,7 +83,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline typename unordered_map<_Key, _Tp, _Hash, _CPred, _Alloc>::size_type
     erase_if(unordered_map<_Key, _Tp, _Hash, _CPred, _Alloc>& __cont,
 	     _Predicate __pred)
-    { return __detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::unordered_map<_Key, _Tp, _Hash, _CPred, _Alloc>& __c
+	= __cont;
+      return __detail::__erase_nodes_if(__c, __pred);
+    }
 
   template<typename _Key, typename _Tp, typename _Hash, typename _CPred,
 	   typename _Alloc, typename _Predicate>
@@ -91,7 +95,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		    size_type
     erase_if(unordered_multimap<_Key, _Tp, _Hash, _CPred, _Alloc>& __cont,
 	     _Predicate __pred)
-    { return __detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::unordered_multimap<_Key, _Tp, _Hash, _CPred, _Alloc>& __c
+	= __cont;
+      return __detail::__erase_nodes_if(__c, __pred);
+    }
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace std
 #endif // C++20
diff --git a/libstdc++-v3/include/std/unordered_set b/libstdc++-v3/include/std/unordered_set
index 1ad93d0031b..3859eeaebd0 100644
--- a/libstdc++-v3/include/std/unordered_set
+++ b/libstdc++-v3/include/std/unordered_set
@@ -83,14 +83,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline typename unordered_set<_Key, _Hash, _CPred, _Alloc>::size_type
     erase_if(unordered_set<_Key, _Hash, _CPred, _Alloc>& __cont,
 	     _Predicate __pred)
-    { return __detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::unordered_set<_Key, _Hash, _CPred, _Alloc>& __c = __cont;
+      return __detail::__erase_nodes_if(__c, __pred);
+    }
 
   template<typename _Key, typename _Hash, typename _CPred, typename _Alloc,
 	   typename _Predicate>
     inline typename unordered_multiset<_Key, _Hash, _CPred, _Alloc>::size_type
     erase_if(unordered_multiset<_Key, _Hash, _CPred, _Alloc>& __cont,
 	     _Predicate __pred)
-    { return __detail::__erase_nodes_if(__cont, __pred); }
+    {
+      _GLIBCXX_STD_C::unordered_multiset<_Key, _Hash, _CPred, _Alloc>& __c
+	= __cont;
+      return __detail::__erase_nodes_if(__c, __pred);
+    }
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace std
 #endif // C++20
diff --git a/libstdc++-v3/include/std/vector b/libstdc++-v3/include/std/vector
index 3b275e249fc..835fa8aeb69 100644
--- a/libstdc++-v3/include/std/vector
+++ b/libstdc++-v3/include/std/vector
@@ -105,28 +105,28 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline typename vector<_Tp, _Alloc>::size_type
     erase_if(vector<_Tp, _Alloc>& __cont, _Predicate __pred)
     {
+      _GLIBCXX_STD_C::vector<_Tp, _Alloc>& __c = __cont;
       using namespace __gnu_cxx;
-      const auto __osz = __cont.size();
-      const auto __end = __cont.end();
-      auto __removed(std::__remove_if(std::__niter_base(__cont.begin()),
-				      std::__niter_base(__end),
+      const auto __osz = __c.size();
+      const auto __end = __c.end();
+      auto __removed(std::__remove_if(__c.begin(), __end,
 				      __ops::__pred_iter(std::ref(__pred))));
-      __cont.erase(std::__niter_wrap(__end, __removed), __end);
-      return __osz - __cont.size();
+      __c.erase(__removed, __end);
+      return __osz - __c.size();
     }
 
   template<typename _Tp, typename _Alloc, typename _Up>
     inline typename vector<_Tp, _Alloc>::size_type
     erase(vector<_Tp, _Alloc>& __cont, const _Up& __value)
     {
+      _GLIBCXX_STD_C::vector<_Tp, _Alloc>& __c = __cont;
       using namespace __gnu_cxx;
-      const auto __osz = __cont.size();
-      const auto __end = __cont.end();
-      auto __removed(std::__remove_if(std::__niter_base(__cont.begin()),
-				      std::__niter_base(__end),
-				      __ops::__iter_equals_val(__value)));
-      __cont.erase(std::__niter_wrap(__end, __removed), __end);
-      return __osz - __cont.size();
+      const auto __osz = __c.size();
+      const auto __end = __c.end();
+      auto __removed = std::__remove_if(__c.begin(), __end,
+					__ops::__iter_equals_val(__value));
+      __c.erase(__removed, __end);
+      return __osz - __c.size();
     }
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace std

Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546]

[François Dumont]

On 07/10/21 4:02 pm, Jonathan Wakely wrote:
> On Thu, 7 Oct 2021 at 07:52, Jonathan Wakely via Libstdc++
> <libstdc++@gcc.gnu.org> wrote:
>> On Thu, 7 Oct 2021, 07:35 François Dumont, <frs.dumont@gmail.com> wrote:
>>
>>> On 02/10/21 2:47 pm, Jonathan Wakely wrote:
>>>
>>>
>>>
>>> On Sat, 2 Oct 2021, 13:02 François Dumont via Libstdc++, <
>>> libstdc++@gcc.gnu.org> wrote:
>>>
>>>> On 01/10/21 9:38 pm, Jonathan Wakely via Libstdc++ wrote:
>>>>> This reduces the preprocessed size of <deque>, <string> and <vector> by
>>>>> not including <bits/stl_algo.h> for std::remove and std::remove_if.
>>>>>
>>>>> Also unwrap iterators using __niter_base, to avoid redundant debug mode
>>>>> checks.
>>>> I don't know if you noticed but the __niter_base is a no-op here.
>>>>
>>> Oh, I didn't notice.
>>>
>>>
>>>> __niter_base unwrap only random access iterators because it is the only
>>>> category for which we know that we have been able to confirm validity or
>>>> not.
>>>>
>>> But these are all random access. I must be missing something.
>>>
>>> It is in a method called '__erases_node_if', I'm not aware of any
>>> node-based container providing random access iterators.
>>>
>> Ah, I thought you meant the deque, string and vector part of the patch,
>> sorry.
>>
>> Moreover I just noticed that if it was really doing anything then you would
>>> be missing the std::__niter_wrap in the __cont.erase(__iter), it just
>>> wouldn't compile.
>>>
>>>
>>>> We would need to introduce another function to do this or specialize in
>>>> some way erase_if for debug containers. I'll try to find a solution.
>>>>
>>> OK, thanks. Maybe we can just leave the checking there. I wanted to avoid
>>> the overhead because we know that the iterator range is valid. Any checks
>>> done on each increment and equality comparison are wasteful, as they will
>>> never fail.
>>>
>>> Yes, that would be better indeed.
>>>
>>> But doing it this way you still have the overhead of the _Safe_iterator
>>> addition to the list of the safe container iterators, so a mutex
>>> lock/unlock.
>>>
>>> I'll try to find out how to get a normal iterator from a safe container
>>> even if in this case we will have to support operations on safe container
>>> with normal iterators.
>>>
>> When we know the type, we could use __cont._GLIBCXX_STD_C::vector<T,
>> Alloc>::begin() to access the base version directly. But for the node
>> containers we have a generic function where we don't know the exact type.
>> Is the _Base typedef accessible? __cont.decltype(__cont)::_Base::begin()
>> would work, but it's ugly.
> The solution is simple:
>
>      erase_if(set<_Key, _Compare, _Alloc>& __cont, _Predicate __pred)
> -    { return __detail::__erase_nodes_if(__cont, __pred); }
> +    {
> +      _GLIBCXX_STD_C::set<_Key, _Compare, _Alloc>& __c = __cont;
> +      return __detail::__erase_nodes_if(__c, __pred);
> +    }
>
>
> i.e. just bind a reference to the non-debug container type. For
> non-debug mode, that is a no-op. For debug mode it binds to the base,
> and the rest of the function works directly on the base, without safe
> iterators.
>
> I'm testing the patch now.
>
Yes, it's a nice approach.

But the problem is that you are going to potentially erase node from the 
container in the back of the safe container. In the end we might have 
valid _Safe_iterator pointing to destroyed nodes.

In fact I am working on a patch to remove the public inheritance betwen 
the Safe container and its normal counterpart to break this kind of 
code. Do you think it would be ok ?

Ideally I would have like to allow a user to access a const reference to 
the underlying "unsafe" container, but I don't think C++ allow this. But 
for your code it would still be considered as invalid.

Here what we need is to get an unsafe iterator from the safe-container 
and pass it back to the safe-container for deletion. This is what I plan 
to work on after getting rid of the public inheritance.

Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546]

[Jonathan Wakely]

On Thu, 7 Oct 2021 at 18:06, François Dumont <frs.dumont@gmail.com> wrote:
>
> On 07/10/21 4:02 pm, Jonathan Wakely wrote:
> > On Thu, 7 Oct 2021 at 07:52, Jonathan Wakely via Libstdc++
> > <libstdc++@gcc.gnu.org> wrote:
> >> On Thu, 7 Oct 2021, 07:35 François Dumont, <frs.dumont@gmail.com> wrote:
> >>
> >>> On 02/10/21 2:47 pm, Jonathan Wakely wrote:
> >>>
> >>>
> >>>
> >>> On Sat, 2 Oct 2021, 13:02 François Dumont via Libstdc++, <
> >>> libstdc++@gcc.gnu.org> wrote:
> >>>
> >>>> On 01/10/21 9:38 pm, Jonathan Wakely via Libstdc++ wrote:
> >>>>> This reduces the preprocessed size of <deque>, <string> and <vector> by
> >>>>> not including <bits/stl_algo.h> for std::remove and std::remove_if.
> >>>>>
> >>>>> Also unwrap iterators using __niter_base, to avoid redundant debug mode
> >>>>> checks.
> >>>> I don't know if you noticed but the __niter_base is a no-op here.
> >>>>
> >>> Oh, I didn't notice.
> >>>
> >>>
> >>>> __niter_base unwrap only random access iterators because it is the only
> >>>> category for which we know that we have been able to confirm validity or
> >>>> not.
> >>>>
> >>> But these are all random access. I must be missing something.
> >>>
> >>> It is in a method called '__erases_node_if', I'm not aware of any
> >>> node-based container providing random access iterators.
> >>>
> >> Ah, I thought you meant the deque, string and vector part of the patch,
> >> sorry.
> >>
> >> Moreover I just noticed that if it was really doing anything then you would
> >>> be missing the std::__niter_wrap in the __cont.erase(__iter), it just
> >>> wouldn't compile.
> >>>
> >>>
> >>>> We would need to introduce another function to do this or specialize in
> >>>> some way erase_if for debug containers. I'll try to find a solution.
> >>>>
> >>> OK, thanks. Maybe we can just leave the checking there. I wanted to avoid
> >>> the overhead because we know that the iterator range is valid. Any checks
> >>> done on each increment and equality comparison are wasteful, as they will
> >>> never fail.
> >>>
> >>> Yes, that would be better indeed.
> >>>
> >>> But doing it this way you still have the overhead of the _Safe_iterator
> >>> addition to the list of the safe container iterators, so a mutex
> >>> lock/unlock.
> >>>
> >>> I'll try to find out how to get a normal iterator from a safe container
> >>> even if in this case we will have to support operations on safe container
> >>> with normal iterators.
> >>>
> >> When we know the type, we could use __cont._GLIBCXX_STD_C::vector<T,
> >> Alloc>::begin() to access the base version directly. But for the node
> >> containers we have a generic function where we don't know the exact type.
> >> Is the _Base typedef accessible? __cont.decltype(__cont)::_Base::begin()
> >> would work, but it's ugly.
> > The solution is simple:
> >
> >      erase_if(set<_Key, _Compare, _Alloc>& __cont, _Predicate __pred)
> > -    { return __detail::__erase_nodes_if(__cont, __pred); }
> > +    {
> > +      _GLIBCXX_STD_C::set<_Key, _Compare, _Alloc>& __c = __cont;
> > +      return __detail::__erase_nodes_if(__c, __pred);
> > +    }
> >
> >
> > i.e. just bind a reference to the non-debug container type. For
> > non-debug mode, that is a no-op. For debug mode it binds to the base,
> > and the rest of the function works directly on the base, without safe
> > iterators.
> >
> > I'm testing the patch now.
> >
> Yes, it's a nice approach.
>
> But the problem is that you are going to potentially erase node from the
> container in the back of the safe container. In the end we might have
> valid _Safe_iterator pointing to destroyed nodes.

Bah. You're right, unfortunately I just pushed the patch.

>
> In fact I am working on a patch to remove the public inheritance betwen
> the Safe container and its normal counterpart to break this kind of
> code. Do you think it would be ok ?

It might break valid uses of the containers in the __gnu_debug namespace.

We document at https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_design.html
that the real container is a public base class of the debug one.


>
> Ideally I would have like to allow a user to access a const reference to
> the underlying "unsafe" container, but I don't think C++ allow this. But
> for your code it would still be considered as invalid.
>
> Here what we need is to get an unsafe iterator from the safe-container
> and pass it back to the safe-container for deletion. This is what I plan
> to work on after getting rid of the public inheritance.

Or maybe we should just leave the debug checks in place, and live with
the overhead. That seems simplest. If we can't remove the overhead
entirely, then we should just let the safe containers correctly track
the changes to the containers. And the simplest way is to just use the
normal interface of the debug containers. Otherwise those very simple
functions get complicated and hard to understand.

Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546]

[François Dumont]

On 07/10/21 7:16 pm, Jonathan Wakely wrote:
> On Thu, 7 Oct 2021 at 18:06, François Dumont <frs.dumont@gmail.com> wrote:
>> On 07/10/21 4:02 pm, Jonathan Wakely wrote:
>>> On Thu, 7 Oct 2021 at 07:52, Jonathan Wakely via Libstdc++
>>> <libstdc++@gcc.gnu.org> wrote:
>>>> On Thu, 7 Oct 2021, 07:35 François Dumont, <frs.dumont@gmail.com> wrote:
>>>>
>>>>> On 02/10/21 2:47 pm, Jonathan Wakely wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Sat, 2 Oct 2021, 13:02 François Dumont via Libstdc++, <
>>>>> libstdc++@gcc.gnu.org> wrote:
>>>>>
>>>>>> On 01/10/21 9:38 pm, Jonathan Wakely via Libstdc++ wrote:
>>>>>>> This reduces the preprocessed size of <deque>, <string> and <vector> by
>>>>>>> not including <bits/stl_algo.h> for std::remove and std::remove_if.
>>>>>>>
>>>>>>> Also unwrap iterators using __niter_base, to avoid redundant debug mode
>>>>>>> checks.
>>>>>> I don't know if you noticed but the __niter_base is a no-op here.
>>>>>>
>>>>> Oh, I didn't notice.
>>>>>
>>>>>
>>>>>> __niter_base unwrap only random access iterators because it is the only
>>>>>> category for which we know that we have been able to confirm validity or
>>>>>> not.
>>>>>>
>>>>> But these are all random access. I must be missing something.
>>>>>
>>>>> It is in a method called '__erases_node_if', I'm not aware of any
>>>>> node-based container providing random access iterators.
>>>>>
>>>> Ah, I thought you meant the deque, string and vector part of the patch,
>>>> sorry.
>>>>
>>>> Moreover I just noticed that if it was really doing anything then you would
>>>>> be missing the std::__niter_wrap in the __cont.erase(__iter), it just
>>>>> wouldn't compile.
>>>>>
>>>>>
>>>>>> We would need to introduce another function to do this or specialize in
>>>>>> some way erase_if for debug containers. I'll try to find a solution.
>>>>>>
>>>>> OK, thanks. Maybe we can just leave the checking there. I wanted to avoid
>>>>> the overhead because we know that the iterator range is valid. Any checks
>>>>> done on each increment and equality comparison are wasteful, as they will
>>>>> never fail.
>>>>>
>>>>> Yes, that would be better indeed.
>>>>>
>>>>> But doing it this way you still have the overhead of the _Safe_iterator
>>>>> addition to the list of the safe container iterators, so a mutex
>>>>> lock/unlock.
>>>>>
>>>>> I'll try to find out how to get a normal iterator from a safe container
>>>>> even if in this case we will have to support operations on safe container
>>>>> with normal iterators.
>>>>>
>>>> When we know the type, we could use __cont._GLIBCXX_STD_C::vector<T,
>>>> Alloc>::begin() to access the base version directly. But for the node
>>>> containers we have a generic function where we don't know the exact type.
>>>> Is the _Base typedef accessible? __cont.decltype(__cont)::_Base::begin()
>>>> would work, but it's ugly.
>>> The solution is simple:
>>>
>>>       erase_if(set<_Key, _Compare, _Alloc>& __cont, _Predicate __pred)
>>> -    { return __detail::__erase_nodes_if(__cont, __pred); }
>>> +    {
>>> +      _GLIBCXX_STD_C::set<_Key, _Compare, _Alloc>& __c = __cont;
>>> +      return __detail::__erase_nodes_if(__c, __pred);
>>> +    }
>>>
>>>
>>> i.e. just bind a reference to the non-debug container type. For
>>> non-debug mode, that is a no-op. For debug mode it binds to the base,
>>> and the rest of the function works directly on the base, without safe
>>> iterators.
>>>
>>> I'm testing the patch now.
>>>
>> Yes, it's a nice approach.
>>
>> But the problem is that you are going to potentially erase node from the
>> container in the back of the safe container. In the end we might have
>> valid _Safe_iterator pointing to destroyed nodes.
> Bah. You're right, unfortunately I just pushed the patch.
>
>> In fact I am working on a patch to remove the public inheritance betwen
>> the Safe container and its normal counterpart to break this kind of
>> code. Do you think it would be ok ?
> It might break valid uses of the containers in the __gnu_debug namespace.

Indeed, it just allowed me to spot for example that the 'using 
_Base::merge' in the safe unordered containers is wrong because it does 
what you intended to do here that is to say modify underlying container 
in the back of the safe one. I am fixing this.


>
> We document at https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_design.html
> that the real container is a public base class of the debug one.

Well, I can change the doc :-)

Do you confirm that you don't want to change this ?

>> Ideally I would have like to allow a user to access a const reference to
>> the underlying "unsafe" container, but I don't think C++ allow this. But
>> for your code it would still be considered as invalid.
>>
>> Here what we need is to get an unsafe iterator from the safe-container
>> and pass it back to the safe-container for deletion. This is what I plan
>> to work on after getting rid of the public inheritance.
> Or maybe we should just leave the debug checks in place, and live with
> the overhead. That seems simplest. If we can't remove the overhead
> entirely, then we should just let the safe containers correctly track
> the changes to the containers. And the simplest way is to just use the
> normal interface of the debug containers. Otherwise those very simple
> functions get complicated and hard to understand.
>
Yes, for now I think it is better to remove any safe-container 
consideration in this code.

Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546]

[Jonathan Wakely]

On Thu, 7 Oct 2021, 18:34 François Dumont, <frs.dumont@gmail.com> wrote:

> On 07/10/21 7:16 pm, Jonathan Wakely wrote:
> > On Thu, 7 Oct 2021 at 18:06, François Dumont <frs.dumont@gmail.com>
> wrote:
> >> On 07/10/21 4:02 pm, Jonathan Wakely wrote:
> >>> On Thu, 7 Oct 2021 at 07:52, Jonathan Wakely via Libstdc++
> >>> <libstdc++@gcc.gnu.org> wrote:
> >>>> On Thu, 7 Oct 2021, 07:35 François Dumont, <frs.dumont@gmail.com>
> wrote:
> >>>>
> >>>>> On 02/10/21 2:47 pm, Jonathan Wakely wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On Sat, 2 Oct 2021, 13:02 François Dumont via Libstdc++, <
> >>>>> libstdc++@gcc.gnu.org> wrote:
> >>>>>
> >>>>>> On 01/10/21 9:38 pm, Jonathan Wakely via Libstdc++ wrote:
> >>>>>>> This reduces the preprocessed size of <deque>, <string> and
> <vector> by
> >>>>>>> not including <bits/stl_algo.h> for std::remove and std::remove_if.
> >>>>>>>
> >>>>>>> Also unwrap iterators using __niter_base, to avoid redundant debug
> mode
> >>>>>>> checks.
> >>>>>> I don't know if you noticed but the __niter_base is a no-op here.
> >>>>>>
> >>>>> Oh, I didn't notice.
> >>>>>
> >>>>>
> >>>>>> __niter_base unwrap only random access iterators because it is the
> only
> >>>>>> category for which we know that we have been able to confirm
> validity or
> >>>>>> not.
> >>>>>>
> >>>>> But these are all random access. I must be missing something.
> >>>>>
> >>>>> It is in a method called '__erases_node_if', I'm not aware of any
> >>>>> node-based container providing random access iterators.
> >>>>>
> >>>> Ah, I thought you meant the deque, string and vector part of the
> patch,
> >>>> sorry.
> >>>>
> >>>> Moreover I just noticed that if it was really doing anything then you
> would
> >>>>> be missing the std::__niter_wrap in the __cont.erase(__iter), it just
> >>>>> wouldn't compile.
> >>>>>
> >>>>>
> >>>>>> We would need to introduce another function to do this or
> specialize in
> >>>>>> some way erase_if for debug containers. I'll try to find a solution.
> >>>>>>
> >>>>> OK, thanks. Maybe we can just leave the checking there. I wanted to
> avoid
> >>>>> the overhead because we know that the iterator range is valid. Any
> checks
> >>>>> done on each increment and equality comparison are wasteful, as they
> will
> >>>>> never fail.
> >>>>>
> >>>>> Yes, that would be better indeed.
> >>>>>
> >>>>> But doing it this way you still have the overhead of the
> _Safe_iterator
> >>>>> addition to the list of the safe container iterators, so a mutex
> >>>>> lock/unlock.
> >>>>>
> >>>>> I'll try to find out how to get a normal iterator from a safe
> container
> >>>>> even if in this case we will have to support operations on safe
> container
> >>>>> with normal iterators.
> >>>>>
> >>>> When we know the type, we could use __cont._GLIBCXX_STD_C::vector<T,
> >>>> Alloc>::begin() to access the base version directly. But for the node
> >>>> containers we have a generic function where we don't know the exact
> type.
> >>>> Is the _Base typedef accessible?
> __cont.decltype(__cont)::_Base::begin()
> >>>> would work, but it's ugly.
> >>> The solution is simple:
> >>>
> >>>       erase_if(set<_Key, _Compare, _Alloc>& __cont, _Predicate __pred)
> >>> -    { return __detail::__erase_nodes_if(__cont, __pred); }
> >>> +    {
> >>> +      _GLIBCXX_STD_C::set<_Key, _Compare, _Alloc>& __c = __cont;
> >>> +      return __detail::__erase_nodes_if(__c, __pred);
> >>> +    }
> >>>
> >>>
> >>> i.e. just bind a reference to the non-debug container type. For
> >>> non-debug mode, that is a no-op. For debug mode it binds to the base,
> >>> and the rest of the function works directly on the base, without safe
> >>> iterators.
> >>>
> >>> I'm testing the patch now.
> >>>
> >> Yes, it's a nice approach.
> >>
> >> But the problem is that you are going to potentially erase node from the
> >> container in the back of the safe container. In the end we might have
> >> valid _Safe_iterator pointing to destroyed nodes.
> > Bah. You're right, unfortunately I just pushed the patch.
> >
> >> In fact I am working on a patch to remove the public inheritance betwen
> >> the Safe container and its normal counterpart to break this kind of
> >> code. Do you think it would be ok ?
> > It might break valid uses of the containers in the __gnu_debug namespace.
>
> Indeed, it just allowed me to spot for example that the 'using
> _Base::merge' in the safe unordered containers is wrong because it does
> what you intended to do here that is to say modify underlying container
> in the back of the safe one. I am fixing this.
>
>
> >
> > We document at
> https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_design.html
> > that the real container is a public base class of the debug one.
>
> Well, I can change the doc :-)
>
> Do you confirm that you don't want to change this ?
>

I'm undecided. One the one hand, having the public base breaks
encapsulation, but on the other hand it's a documented part of the API.

It would be good to know how many users actually rely on it, before we
change the guarantees.


> >> Ideally I would have like to allow a user to access a const reference to
> >> the underlying "unsafe" container, but I don't think C++ allow this. But
> >> for your code it would still be considered as invalid.
> >>
> >> Here what we need is to get an unsafe iterator from the safe-container
> >> and pass it back to the safe-container for deletion. This is what I plan
> >> to work on after getting rid of the public inheritance.
> > Or maybe we should just leave the debug checks in place, and live with
> > the overhead. That seems simplest. If we can't remove the overhead
> > entirely, then we should just let the safe containers correctly track
> > the changes to the containers. And the simplest way is to just use the
> > normal interface of the debug containers. Otherwise those very simple
> > functions get complicated and hard to understand.
> >
> Yes, for now I think it is better to remove any safe-container
> consideration in this code.
>

OK, I'll remove it.

Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546]

[François Dumont]

On 07/10/21 8:34 pm, Jonathan Wakely wrote:
>
>
> On Thu, 7 Oct 2021, 18:34 François Dumont, <frs.dumont@gmail.com 
> <mailto:frs.dumont@gmail.com>> wrote:
>
>     On 07/10/21 7:16 pm, Jonathan Wakely wrote:
>     > On Thu, 7 Oct 2021 at 18:06, François Dumont
>     <frs.dumont@gmail.com <mailto:frs.dumont@gmail.com>> wrote:
>     >> On 07/10/21 4:02 pm, Jonathan Wakely wrote:
>     >>> On Thu, 7 Oct 2021 at 07:52, Jonathan Wakely via Libstdc++
>     >>> <libstdc++@gcc.gnu.org <mailto:libstdc%2B%2B@gcc.gnu.org>> wrote:
>     >>>> On Thu, 7 Oct 2021, 07:35 François Dumont,
>     <frs.dumont@gmail.com <mailto:frs.dumont@gmail.com>> wrote:
>     >>>>
>     >>>>> On 02/10/21 2:47 pm, Jonathan Wakely wrote:
>     >>>>>
>     >>>>>
>     >>>>>
>     >>>>> On Sat, 2 Oct 2021, 13:02 François Dumont via Libstdc++, <
>     >>>>> libstdc++@gcc.gnu.org <mailto:libstdc%2B%2B@gcc.gnu.org>> wrote:
>     >>>>>
>     >>>>>> On 01/10/21 9:38 pm, Jonathan Wakely via Libstdc++ wrote:
>     >>>>>>> This reduces the preprocessed size of <deque>, <string>
>     and <vector> by
>     >>>>>>> not including <bits/stl_algo.h> for std::remove and
>     std::remove_if.
>     >>>>>>>
>     >>>>>>> Also unwrap iterators using __niter_base, to avoid
>     redundant debug mode
>     >>>>>>> checks.
>     >>>>>> I don't know if you noticed but the __niter_base is a no-op
>     here.
>     >>>>>>
>     >>>>> Oh, I didn't notice.
>     >>>>>
>     >>>>>
>     >>>>>> __niter_base unwrap only random access iterators because it
>     is the only
>     >>>>>> category for which we know that we have been able to
>     confirm validity or
>     >>>>>> not.
>     >>>>>>
>     >>>>> But these are all random access. I must be missing something.
>     >>>>>
>     >>>>> It is in a method called '__erases_node_if', I'm not aware
>     of any
>     >>>>> node-based container providing random access iterators.
>     >>>>>
>     >>>> Ah, I thought you meant the deque, string and vector part of
>     the patch,
>     >>>> sorry.
>     >>>>
>     >>>> Moreover I just noticed that if it was really doing anything
>     then you would
>     >>>>> be missing the std::__niter_wrap in the
>     __cont.erase(__iter), it just
>     >>>>> wouldn't compile.
>     >>>>>
>     >>>>>
>     >>>>>> We would need to introduce another function to do this or
>     specialize in
>     >>>>>> some way erase_if for debug containers. I'll try to find a
>     solution.
>     >>>>>>
>     >>>>> OK, thanks. Maybe we can just leave the checking there. I
>     wanted to avoid
>     >>>>> the overhead because we know that the iterator range is
>     valid. Any checks
>     >>>>> done on each increment and equality comparison are wasteful,
>     as they will
>     >>>>> never fail.
>     >>>>>
>     >>>>> Yes, that would be better indeed.
>     >>>>>
>     >>>>> But doing it this way you still have the overhead of the
>     _Safe_iterator
>     >>>>> addition to the list of the safe container iterators, so a mutex
>     >>>>> lock/unlock.
>     >>>>>
>     >>>>> I'll try to find out how to get a normal iterator from a
>     safe container
>     >>>>> even if in this case we will have to support operations on
>     safe container
>     >>>>> with normal iterators.
>     >>>>>
>     >>>> When we know the type, we could use
>     __cont._GLIBCXX_STD_C::vector<T,
>     >>>> Alloc>::begin() to access the base version directly. But for
>     the node
>     >>>> containers we have a generic function where we don't know the
>     exact type.
>     >>>> Is the _Base typedef accessible?
>     __cont.decltype(__cont)::_Base::begin()
>     >>>> would work, but it's ugly.
>     >>> The solution is simple:
>     >>>
>     >>>       erase_if(set<_Key, _Compare, _Alloc>& __cont, _Predicate
>     __pred)
>     >>> -    { return __detail::__erase_nodes_if(__cont, __pred); }
>     >>> +    {
>     >>> +      _GLIBCXX_STD_C::set<_Key, _Compare, _Alloc>& __c = __cont;
>     >>> +      return __detail::__erase_nodes_if(__c, __pred);
>     >>> +    }
>     >>>
>     >>>
>     >>> i.e. just bind a reference to the non-debug container type. For
>     >>> non-debug mode, that is a no-op. For debug mode it binds to
>     the base,
>     >>> and the rest of the function works directly on the base,
>     without safe
>     >>> iterators.
>     >>>
>     >>> I'm testing the patch now.
>     >>>
>     >> Yes, it's a nice approach.
>     >>
>     >> But the problem is that you are going to potentially erase node
>     from the
>     >> container in the back of the safe container. In the end we
>     might have
>     >> valid _Safe_iterator pointing to destroyed nodes.
>     > Bah. You're right, unfortunately I just pushed the patch.
>     >
>     >> In fact I am working on a patch to remove the public
>     inheritance betwen
>     >> the Safe container and its normal counterpart to break this kind of
>     >> code. Do you think it would be ok ?
>     > It might break valid uses of the containers in the __gnu_debug
>     namespace.
>
>     Indeed, it just allowed me to spot for example that the 'using
>     _Base::merge' in the safe unordered containers is wrong because it
>     does
>     what you intended to do here that is to say modify underlying
>     container
>     in the back of the safe one. I am fixing this.
>
>
>     >
>     > We document at
>     https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_design.html
>     <https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_design.html>
>     > that the real container is a public base class of the debug one.
>
>     Well, I can change the doc :-)
>
>     Do you confirm that you don't want to change this ?
>
>
> I'm undecided. One the one hand, having the public base breaks 
> encapsulation, but on the other hand it's a documented part of the API.
>
> It would be good to know how many users actually rely on it, before we 
> change the guarantees.

Don't worry, I won't do it.

We live with that until now and moreover I am planing to do something 
like this:

     template<typename _Container, typename _UnsafeContainer, typename 
_Predicate>
       typename _Container::size_type
       __erase_nodes_if(_Container& __cont, _UnsafeContainer& __ucont, 
_Predicate __pred)

and in the implementation we will do something like: 
__cont.erase(__ucont.begin());

So we'll need the inheritance.

People will just have to be careful, I'll check that documentation is 
clear on that.