[PATCH] Fix _GLIBCXX_DEBUG container allocator aware move constructors

[PATCH] Fix _GLIBCXX_DEBUG container allocator aware move constructors

[François Dumont]

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

I review the allocator aware move constructors of _GLIBCXX_DEBUG containers.

I think the recently added __gnu_debug basic_string one is also missing 
the rvalue reference, no ?

     libstdc++: _GLIBCXX_DEBUG Fix allocator aware move constructor

     Fix several allocator aware move construtor in _GLIBCXX_DEBUG
     containers.

     libstdc++-v3/ChangeLog:
             * include/debug/forward_list
             (forward_list(forward_list&&, const allocator_type&)): Add 
noexcept qualification.
             * include/debug/list (list(list&&, const allocator_type&)): 
Likewise and add
             call to safe container allocator aware move constructor.
             * include/debug/string (basic_string(basic_string&&, const 
_Allocator&)):
             Check base type allocator aware more constructor.
             * include/debug/vector (vector(vector&&, const 
allocator_type&)):
             Fix noexcept qualification.
             * 
testsuite/23_containers/forward_list/cons/noexcept_move_construct.cc:
             Add allocator aware move constructor noexceot qualification 
check.
             * 
testsuite/23_containers/list/cons/noexcept_move_construct.cc: Likewise.

Tested under linux x86_64.

Ok to commit ?

François


[-- Attachment #2: debug_noexcept-cons.patch --]
[-- Type: text/x-patch, Size: 4034 bytes --]

diff --git a/libstdc++-v3/include/debug/forward_list b/libstdc++-v3/include/debug/forward_list
index db46705cc71..d631d53c62e 100644
--- a/libstdc++-v3/include/debug/forward_list
+++ b/libstdc++-v3/include/debug/forward_list
@@ -239,8 +239,11 @@ namespace __debug
       { }
 
       forward_list(forward_list&& __list, const allocator_type& __al)
-	: _Safe(std::move(__list._M_safe()), __al),
-	  _Base(std::move(__list._M_base()), __al)
+	noexcept(
+	  std::is_nothrow_constructible<_Base,
+	    _Base&&, const allocator_type&>::value )
+      : _Safe(std::move(__list._M_safe()), __al),
+	_Base(std::move(__list._M_base()), __al)
       { }
 
       explicit
diff --git a/libstdc++-v3/include/debug/list b/libstdc++-v3/include/debug/list
index 06938899253..7b6e15bee07 100644
--- a/libstdc++-v3/include/debug/list
+++ b/libstdc++-v3/include/debug/list
@@ -119,7 +119,11 @@ namespace __debug
       : _Base(__x, __a) { }
 
       list(list&& __x, const allocator_type& __a)
-      : _Base(std::move(__x), __a) { }
+	noexcept(
+	  std::is_nothrow_constructible<_Base,
+	    _Base&&, const allocator_type&>::value )
+      : _Safe(std::move(__x._M_safe()), __a),
+	_Base(std::move(__x._M_base()), __a) { }
 #endif
 
       explicit
diff --git a/libstdc++-v3/include/debug/string b/libstdc++-v3/include/debug/string
index 8744a55be64..038b06a3097 100644
--- a/libstdc++-v3/include/debug/string
+++ b/libstdc++-v3/include/debug/string
@@ -159,7 +159,7 @@ namespace __gnu_debug
 
       basic_string(basic_string&& __s, const _Allocator& __a)
       noexcept(
-	std::is_nothrow_constructible<_Base, _Base, const _Allocator&>::value )
+	std::is_nothrow_constructible<_Base, _Base&&, const _Allocator&>::value )
       : _Safe(std::move(__s._M_safe()), __a),
 	_Base(std::move(__s._M_base()), __a)
       { }
diff --git a/libstdc++-v3/include/debug/vector b/libstdc++-v3/include/debug/vector
index df179cbbfea..6acf3d3dbf4 100644
--- a/libstdc++-v3/include/debug/vector
+++ b/libstdc++-v3/include/debug/vector
@@ -217,8 +217,9 @@ namespace __debug
       : _Base(__x, __a) { }
 
       vector(vector&& __x, const allocator_type& __a)
-      noexcept(noexcept(
-	_Base(std::declval<_Base&&>()), std::declval<const allocator_type&>()))
+      noexcept(
+	std::is_nothrow_constructible<_Base,
+	  _Base&&, const allocator_type&>::value )
       : _Safe(std::move(__x._M_safe()), __a),
 	_Base(std::move(__x._M_base()), __a),
 	_Safe_vector(std::move(__x)) { }
diff --git a/libstdc++-v3/testsuite/23_containers/forward_list/cons/noexcept_move_construct.cc b/libstdc++-v3/testsuite/23_containers/forward_list/cons/noexcept_move_construct.cc
index 96f3876e4f6..6ca9e96ab4a 100644
--- a/libstdc++-v3/testsuite/23_containers/forward_list/cons/noexcept_move_construct.cc
+++ b/libstdc++-v3/testsuite/23_containers/forward_list/cons/noexcept_move_construct.cc
@@ -23,4 +23,8 @@
 
 typedef std::forward_list<int> fltype;
 
-static_assert(std::is_nothrow_move_constructible<fltype>::value, "Error");
+static_assert( std::is_nothrow_move_constructible<fltype>::value,
+	       "noexcept move constructor" );
+static_assert( std::is_nothrow_constructible<fltype,
+	       fltype&&, const typename fltype::allocator_type&>::value,
+	       "noexcept move constructor with allocator" );
diff --git a/libstdc++-v3/testsuite/23_containers/list/cons/noexcept_move_construct.cc b/libstdc++-v3/testsuite/23_containers/list/cons/noexcept_move_construct.cc
index 5a2de10cf09..63a33a4991b 100644
--- a/libstdc++-v3/testsuite/23_containers/list/cons/noexcept_move_construct.cc
+++ b/libstdc++-v3/testsuite/23_containers/list/cons/noexcept_move_construct.cc
@@ -23,4 +23,8 @@
 
 typedef std::list<int> ltype;
 
-static_assert(std::is_nothrow_move_constructible<ltype>::value, "Error");
+static_assert( std::is_nothrow_move_constructible<ltype>::value,
+	       "noexcept move constructor" );
+static_assert( std::is_nothrow_constructible<ltype,
+	       ltype&&, const typename ltype::allocator_type&>::value,
+	       "noexcept move constructor with allocator" );

Re: [PATCH] Fix _GLIBCXX_DEBUG container allocator aware move constructors

[Jonathan Wakely]

On 26/03/21 21:41 +0100, François Dumont via Libstdc++ wrote:
>I review the allocator aware move constructors of _GLIBCXX_DEBUG containers.
>
>I think the recently added __gnu_debug basic_string one is also 
>missing the rvalue reference, no ?

You mean is_nothrow_constructible<Base, Base&&, const Alloc&> instead
of is_nothrow_constructible<Base,Base, const Alloc&>?

No, it's not missing. They mean the exact same thing: Base is an
rvalue, Base&& is an rvalue, and Base& is an lvalue. Writing Base&&
instead of Base is just extra noise that adds no value.


>    libstdc++: _GLIBCXX_DEBUG Fix allocator aware move constructor

The correct term is "allocator-extended move constructor". And is it
really a "fix"? The standard doesn't require a noexcept-specifier on
those constructors, so what we have now is not incorrect, just
suboptimal.

>    Fix several allocator aware move construtor in _GLIBCXX_DEBUG
>    containers.
>
>    libstdc++-v3/ChangeLog:
>            * include/debug/forward_list
>            (forward_list(forward_list&&, const allocator_type&)): Add 
>noexcept qualification.
>            * include/debug/list (list(list&&, const 
>allocator_type&)): Likewise and add
>            call to safe container allocator aware move constructor.
>            * include/debug/string (basic_string(basic_string&&, const 
>_Allocator&)):
>            Check base type allocator aware more constructor.
>            * include/debug/vector (vector(vector&&, const 
>allocator_type&)):
>            Fix noexcept qualification.
>            * 
>testsuite/23_containers/forward_list/cons/noexcept_move_construct.cc:
>            Add allocator aware move constructor noexceot 
>qualification check.
>            * 
>testsuite/23_containers/list/cons/noexcept_move_construct.cc: 
>Likewise.
>
>Tested under linux x86_64.
>
>Ok to commit ?

OK, thanks.

Re: [PATCH] Fix _GLIBCXX_DEBUG container allocator aware move constructors

[François Dumont]

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

On 27/03/21 10:07 pm, Jonathan Wakely wrote:
> On 26/03/21 21:41 +0100, François Dumont via Libstdc++ wrote:
>> I review the allocator aware move constructors of _GLIBCXX_DEBUG 
>> containers.
>>
>> I think the recently added __gnu_debug basic_string one is also 
>> missing the rvalue reference, no ?
>
> You mean is_nothrow_constructible<Base, Base&&, const Alloc&> instead
> of is_nothrow_constructible<Base,Base, const Alloc&>?
>
> No, it's not missing. They mean the exact same thing: Base is an
> rvalue, Base&& is an rvalue, and Base& is an lvalue. Writing Base&&
> instead of Base is just extra noise that adds no value.
>
Ok, noted.
>
>>     libstdc++: _GLIBCXX_DEBUG Fix allocator aware move constructor
>
> The correct term is "allocator-extended move constructor". And is it
> really a "fix"? The standard doesn't require a noexcept-specifier on
> those constructors, so what we have now is not incorrect, just
> suboptimal.

For the debug list it is a fix because we were missing the _Safe base 
class call. For the vector it is also a fix even if a minor one.

>
>>     Fix several allocator aware move construtor in _GLIBCXX_DEBUG
>>     containers.
>>
>>     libstdc++-v3/ChangeLog:
>>             * include/debug/forward_list
>>             (forward_list(forward_list&&, const allocator_type&)): 
>> Add noexcept qualification.
>>             * include/debug/list (list(list&&, const 
>> allocator_type&)): Likewise and add
>>             call to safe container allocator aware move constructor.
>>             * include/debug/string (basic_string(basic_string&&, 
>> const _Allocator&)):
>>             Check base type allocator aware more constructor.
>>             * include/debug/vector (vector(vector&&, const 
>> allocator_type&)):
>>             Fix noexcept qualification.
>>             * 
>> testsuite/23_containers/forward_list/cons/noexcept_move_construct.cc:
>>             Add allocator aware move constructor noexceot 
>> qualification check.
>>             * 
>> testsuite/23_containers/list/cons/noexcept_move_construct.cc: Likewise.
>>
>> Tested under linux x86_64.
>>
>> Ok to commit ?
>
> OK, thanks.
>
>
I eventually committed the attached patch.

If you are still interested by _GLIBCXX_DEBUG fixes there is still this one:

https://gcc.gnu.org/pipermail/libstdc++/2021-March/052176.html

Thanks,

François


[-- Attachment #2: debug_noexcept-cons.patch --]
[-- Type: text/x-patch, Size: 3452 bytes --]

diff --git a/libstdc++-v3/include/debug/forward_list b/libstdc++-v3/include/debug/forward_list
index db46705cc71..16f0531dce7 100644
--- a/libstdc++-v3/include/debug/forward_list
+++ b/libstdc++-v3/include/debug/forward_list
@@ -239,8 +239,11 @@ namespace __debug
       { }
 
       forward_list(forward_list&& __list, const allocator_type& __al)
-	: _Safe(std::move(__list._M_safe()), __al),
-	  _Base(std::move(__list._M_base()), __al)
+	noexcept(
+	  std::is_nothrow_constructible<_Base,
+	    _Base, const allocator_type&>::value )
+      : _Safe(std::move(__list._M_safe()), __al),
+	_Base(std::move(__list._M_base()), __al)
       { }
 
       explicit
diff --git a/libstdc++-v3/include/debug/list b/libstdc++-v3/include/debug/list
index 06938899253..01fe43fc7df 100644
--- a/libstdc++-v3/include/debug/list
+++ b/libstdc++-v3/include/debug/list
@@ -119,7 +119,11 @@ namespace __debug
       : _Base(__x, __a) { }
 
       list(list&& __x, const allocator_type& __a)
-      : _Base(std::move(__x), __a) { }
+	noexcept(
+	  std::is_nothrow_constructible<_Base,
+	    _Base, const allocator_type&>::value )
+      : _Safe(std::move(__x._M_safe()), __a),
+	_Base(std::move(__x._M_base()), __a) { }
 #endif
 
       explicit
diff --git a/libstdc++-v3/include/debug/vector b/libstdc++-v3/include/debug/vector
index df179cbbfea..987bba17c2b 100644
--- a/libstdc++-v3/include/debug/vector
+++ b/libstdc++-v3/include/debug/vector
@@ -217,8 +217,9 @@ namespace __debug
       : _Base(__x, __a) { }
 
       vector(vector&& __x, const allocator_type& __a)
-      noexcept(noexcept(
-	_Base(std::declval<_Base&&>()), std::declval<const allocator_type&>()))
+      noexcept(
+	std::is_nothrow_constructible<_Base,
+	  _Base, const allocator_type&>::value )
       : _Safe(std::move(__x._M_safe()), __a),
 	_Base(std::move(__x._M_base()), __a),
 	_Safe_vector(std::move(__x)) { }
diff --git a/libstdc++-v3/testsuite/23_containers/forward_list/cons/noexcept_move_construct.cc b/libstdc++-v3/testsuite/23_containers/forward_list/cons/noexcept_move_construct.cc
index 96f3876e4f6..0eb5a5cdbba 100644
--- a/libstdc++-v3/testsuite/23_containers/forward_list/cons/noexcept_move_construct.cc
+++ b/libstdc++-v3/testsuite/23_containers/forward_list/cons/noexcept_move_construct.cc
@@ -23,4 +23,8 @@
 
 typedef std::forward_list<int> fltype;
 
-static_assert(std::is_nothrow_move_constructible<fltype>::value, "Error");
+static_assert( std::is_nothrow_move_constructible<fltype>::value,
+	       "noexcept move constructor" );
+static_assert( std::is_nothrow_constructible<fltype,
+	       fltype, const typename fltype::allocator_type&>::value,
+	       "noexcept move constructor with allocator" );
diff --git a/libstdc++-v3/testsuite/23_containers/list/cons/noexcept_move_construct.cc b/libstdc++-v3/testsuite/23_containers/list/cons/noexcept_move_construct.cc
index 5a2de10cf09..858a0d76333 100644
--- a/libstdc++-v3/testsuite/23_containers/list/cons/noexcept_move_construct.cc
+++ b/libstdc++-v3/testsuite/23_containers/list/cons/noexcept_move_construct.cc
@@ -23,4 +23,8 @@
 
 typedef std::list<int> ltype;
 
-static_assert(std::is_nothrow_move_constructible<ltype>::value, "Error");
+static_assert( std::is_nothrow_move_constructible<ltype>::value,
+	       "noexcept move constructor" );
+static_assert( std::is_nothrow_constructible<ltype,
+	       ltype, const typename ltype::allocator_type&>::value,
+	       "noexcept move constructor with allocator" );