* [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] @ 2021-10-01 19:38 Jonathan Wakely 2021-10-02 12:01 ` François Dumont 0 siblings, 1 reply; 12+ messages in thread From: Jonathan Wakely @ 2021-10-01 19:38 UTC (permalink / raw) To: libstdc++, gcc-patches [-- 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] 2021-10-01 19:38 [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] Jonathan Wakely @ 2021-10-02 12:01 ` François Dumont 2021-10-02 12:47 ` Jonathan Wakely 0 siblings, 1 reply; 12+ messages in thread From: François Dumont @ 2021-10-02 12:01 UTC (permalink / raw) To: libstdc++ 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] 2021-10-02 12:01 ` François Dumont @ 2021-10-02 12:47 ` Jonathan Wakely 2021-10-07 6:35 ` François Dumont 0 siblings, 1 reply; 12+ messages in thread From: Jonathan Wakely @ 2021-10-02 12:47 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++ 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 > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] 2021-10-02 12:47 ` Jonathan Wakely @ 2021-10-07 6:35 ` François Dumont 2021-10-07 6:51 ` Jonathan Wakely 0 siblings, 1 reply; 12+ messages in thread From: François Dumont @ 2021-10-07 6:35 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++ 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] 2021-10-07 6:35 ` François Dumont @ 2021-10-07 6:51 ` Jonathan Wakely 2021-10-07 14:02 ` Jonathan Wakely 0 siblings, 1 reply; 12+ messages in thread From: Jonathan Wakely @ 2021-10-07 6:51 UTC (permalink / raw) To: François Dumont; +Cc: libstdc++ 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] 2021-10-07 6:51 ` Jonathan Wakely @ 2021-10-07 14:02 ` Jonathan Wakely 2021-10-07 17:05 ` [committed] libstdc++: Avoid debug checks in uniform container erasure functions Jonathan Wakely 2021-10-07 17:06 ` [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] François Dumont 0 siblings, 2 replies; 12+ messages in thread From: Jonathan Wakely @ 2021-10-07 14:02 UTC (permalink / raw) To: Jonathan Wakely; +Cc: François Dumont, libstdc++ 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [committed] libstdc++: Avoid debug checks in uniform container erasure functions 2021-10-07 14:02 ` Jonathan Wakely @ 2021-10-07 17:05 ` Jonathan Wakely 2021-10-07 17:06 ` [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] François Dumont 1 sibling, 0 replies; 12+ messages in thread From: Jonathan Wakely @ 2021-10-07 17:05 UTC (permalink / raw) To: Jonathan Wakely; +Cc: François Dumont, libstdc++ [-- 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] 2021-10-07 14:02 ` Jonathan Wakely 2021-10-07 17:05 ` [committed] libstdc++: Avoid debug checks in uniform container erasure functions Jonathan Wakely @ 2021-10-07 17:06 ` François Dumont 2021-10-07 17:16 ` Jonathan Wakely 1 sibling, 1 reply; 12+ messages in thread From: François Dumont @ 2021-10-07 17:06 UTC (permalink / raw) To: Jonathan Wakely, Jonathan Wakely; +Cc: libstdc++ 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] 2021-10-07 17:06 ` [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] François Dumont @ 2021-10-07 17:16 ` Jonathan Wakely 2021-10-07 17:34 ` François Dumont 0 siblings, 1 reply; 12+ messages in thread From: Jonathan Wakely @ 2021-10-07 17:16 UTC (permalink / raw) To: François Dumont; +Cc: Jonathan Wakely, libstdc++ 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] 2021-10-07 17:16 ` Jonathan Wakely @ 2021-10-07 17:34 ` François Dumont 2021-10-07 18:34 ` Jonathan Wakely 0 siblings, 1 reply; 12+ messages in thread From: François Dumont @ 2021-10-07 17:34 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Jonathan Wakely, libstdc++ 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] 2021-10-07 17:34 ` François Dumont @ 2021-10-07 18:34 ` Jonathan Wakely 2021-10-07 19:51 ` François Dumont 0 siblings, 1 reply; 12+ messages in thread From: Jonathan Wakely @ 2021-10-07 18:34 UTC (permalink / raw) To: François Dumont; +Cc: Jonathan Wakely, libstdc++ 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] 2021-10-07 18:34 ` Jonathan Wakely @ 2021-10-07 19:51 ` François Dumont 0 siblings, 0 replies; 12+ messages in thread From: François Dumont @ 2021-10-07 19:51 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Jonathan Wakely, libstdc++ 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-10-07 19:51 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-01 19:38 [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] Jonathan Wakely 2021-10-02 12:01 ` François Dumont 2021-10-02 12:47 ` Jonathan Wakely 2021-10-07 6:35 ` François Dumont 2021-10-07 6:51 ` Jonathan Wakely 2021-10-07 14:02 ` Jonathan Wakely 2021-10-07 17:05 ` [committed] libstdc++: Avoid debug checks in uniform container erasure functions Jonathan Wakely 2021-10-07 17:06 ` [committed] libstdc++: Reduce header dependencies for C++20 std::erase [PR92546] François Dumont 2021-10-07 17:16 ` Jonathan Wakely 2021-10-07 17:34 ` François Dumont 2021-10-07 18:34 ` Jonathan Wakely 2021-10-07 19:51 ` François Dumont
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).