diff mbox series

Relax std::move_if_noexcept for std::pair

Message ID 366bf0a3-2daf-d5c5-7a09-f1260c9c405f@gmail.com
State New
Headers show
Series Relax std::move_if_noexcept for std::pair | expand

Commit Message

François Dumont Dec. 20, 2018, 6:29 a.m. UTC 
Hi

     I eventually find out what was the problem with the 
std::move_if_noexcept within associative containers.

     The std::pair move default constructor might not move both first 
and second member. If any is not moveable it will just copy it. And then 
the noexcept qualification of the copy constructor will participate in 
the noexcept qualification of the std::pair move constructor. So 
std::move_if_noexcept can eventually decide to not use move because a 
_copy_ constructor not noexcept qualified.

     This is why I am partially specializing __move_if_noexcept_cond. As 
there doesn't seem to exist any Standard meta function to find out if 
move will take place I resort using std::is_const as in this case for 
sure the compiler won't call the move constructor.

     Note that I find __move_if_noexcept_cond very counter-intuitive 
cause it says if the move semantic should _not_ be used.

     I am submitting this now cause it might be consider as a bug even 
if the end result is just that it pessimizes the occasion to use move 
semantic rather than copy.

     * include/bits/stl_pair.h (__move_if_noexcept_cond<pair<>>): New
     partial specialization.
     * testsuite/20_util/move_if_noexcept/1.cc (test02): New.
     * testsuite/23_containers/unordered_map/allocator/move_assign.cc
     (test03): New.

     Tested under Linux x86_64 normal mode.

François

Comments

Ville Voutilainen Dec. 20, 2018, 8:04 a.m. UTC | #1 
On Thu, 20 Dec 2018 at 08:29, François Dumont <frs.dumont@gmail.com> wrote:
>
> Hi
>
>      I eventually find out what was the problem with the
> std::move_if_noexcept within associative containers.
>
>      The std::pair move default constructor might not move both first
> and second member. If any is not moveable it will just copy it. And then

..as it should..

> the noexcept qualification of the copy constructor will participate in
> the noexcept qualification of the std::pair move constructor. So
> std::move_if_noexcept can eventually decide to not use move because a
> _copy_ constructor not noexcept qualified.

..and again, as it should.

>      This is why I am partially specializing __move_if_noexcept_cond. As
> there doesn't seem to exist any Standard meta function to find out if
> move will take place I resort using std::is_const as in this case for
> sure the compiler won't call the move constructor.

That seems wrong; just because a type is or is not const has nothing
to do whether
it's nothrow_move_constructible.

I don't understand what problem this is solving, and how it's not
introducing new problems.
François Dumont Dec. 20, 2018, 9:53 p.m. UTC | #2 
On 12/20/18 9:04 AM, Ville Voutilainen wrote:
> On Thu, 20 Dec 2018 at 08:29, François Dumont <frs.dumont@gmail.com> wrote:
>> Hi
>>
>>       I eventually find out what was the problem with the
>> std::move_if_noexcept within associative containers.
>>
>>       The std::pair move default constructor might not move both first
>> and second member. If any is not moveable it will just copy it. And then
> ..as it should..
>
>> the noexcept qualification of the copy constructor will participate in
>> the noexcept qualification of the std::pair move constructor. So
>> std::move_if_noexcept can eventually decide to not use move because a
>> _copy_ constructor not noexcept qualified.
> ..and again, as it should.
>
>>       This is why I am partially specializing __move_if_noexcept_cond. As
>> there doesn't seem to exist any Standard meta function to find out if
>> move will take place I resort using std::is_const as in this case for
>> sure the compiler won't call the move constructor.
> That seems wrong; just because a type is or is not const has nothing
> to do whether
> it's nothrow_move_constructible.

Indeed, I am not changing that.


>
> I don't understand what problem this is solving, and how it's not
> introducing new problems.
>
The problem I am trying to solve is shown by the tests I have adapted. 
Allow more move semantic in associative container where key are stored 
as const.

But if I make counter_type copy constructor noexcept then I also get the 
move on the pair.second instance, great. I am just surprise to have to 
make a copy constructor noexcept to have std::move_if_noexcept work as I 
expect.

I think I just need to understand why we need std::move_if_noexcept in 
unordered containers or even rb_tree. Couldn't we just use std::move ? I 
don't understand what we are trying to avoid with this noexcept check.

François
Ville Voutilainen Dec. 20, 2018, 10:19 p.m. UTC | #3 
On Thu, 20 Dec 2018 at 23:53, François Dumont <frs.dumont@gmail.com> wrote:
> >>       This is why I am partially specializing __move_if_noexcept_cond. As
> >> there doesn't seem to exist any Standard meta function to find out if
> >> move will take place I resort using std::is_const as in this case for
> >> sure the compiler won't call the move constructor.
> > That seems wrong; just because a type is or is not const has nothing
> > to do whether
> > it's nothrow_move_constructible.
>
> Indeed, I am not changing that.

Well, if you're not changing that, then I have no idea what is_const
is doing in your patch. :)

> > I don't understand what problem this is solving, and how it's not
> > introducing new problems.
> >
> The problem I am trying to solve is shown by the tests I have adapted.
> Allow more move semantic in associative container where key are stored
> as const.

I'm not sure what you're trying to achieve is doable. The element of
an associative container is a pair,
and it has pair's semantics. It's also a pair<const Key, Value>, and
has those semantics. Those semantics
are observable.

> But if I make counter_type copy constructor noexcept then I also get the
> move on the pair.second instance, great. I am just surprise to have to
> make a copy constructor noexcept to have std::move_if_noexcept work as I
> expect.

Well, move construction/assignment via std::move or
std::move_if_noexcept is not necessarily
going to invoke just move constructors. Especially not for subobjects,
like pair's members.

> I think I just need to understand why we need std::move_if_noexcept in
> unordered containers or even rb_tree. Couldn't we just use std::move ? I
> don't understand what we are trying to avoid with this noexcept check.

We are trying to avoid data corruption on exceptions; if you move a
subobject and have to copy another,
and then that copy operation throws, you can't reliably move the
already-moved subobject back. See
http://open-std.org/JTC1/SC22/WG21/docs/papers/2010/n3050.html
and also
http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2009/n2855.html
Jonathan Wakely Dec. 21, 2018, 1:30 p.m. UTC | #4 
On 20/12/18 22:53 +0100, François Dumont wrote:
>On 12/20/18 9:04 AM, Ville Voutilainen wrote:
>>On Thu, 20 Dec 2018 at 08:29, François Dumont <frs.dumont@gmail.com> wrote:
>>>Hi
>>>
>>>      I eventually find out what was the problem with the
>>>std::move_if_noexcept within associative containers.
>>>
>>>      The std::pair move default constructor might not move both first
>>>and second member. If any is not moveable it will just copy it. And then
>>..as it should..
>>
>>>the noexcept qualification of the copy constructor will participate in
>>>the noexcept qualification of the std::pair move constructor. So
>>>std::move_if_noexcept can eventually decide to not use move because a
>>>_copy_ constructor not noexcept qualified.
>>..and again, as it should.
>>
>>>      This is why I am partially specializing __move_if_noexcept_cond. As
>>>there doesn't seem to exist any Standard meta function to find out if
>>>move will take place I resort using std::is_const as in this case for
>>>sure the compiler won't call the move constructor.
>>That seems wrong; just because a type is or is not const has nothing
>>to do whether
>>it's nothrow_move_constructible.
>
>Indeed, I am not changing that.
>
>
>>
>>I don't understand what problem this is solving, and how it's not
>>introducing new problems.
>>
>The problem I am trying to solve is shown by the tests I have adapted. 
>Allow more move semantic in associative container where key are stored 
>as const.

I'm not convinced that's a desirable property, especially not if it
needs changes to move_if_noexcept.

>But if I make counter_type copy constructor noexcept then I also get 
>the move on the pair.second instance, great. I am just surprise to 
>have to make a copy constructor noexcept to have std::move_if_noexcept 
>work as I expect.

Because the move constructor of pair<const T, U> will copy the first
element not move it, because you can't move from a const object. If
the T(const T&) constructor is noexcept, and the U(U&&) constructor is
also noexcept, then the pair<const T, U> move constructor is noexcept.
The move constructor's exception specification depends on the
exception specifications of whichever constructors it invokes for its
members.

>I think I just need to understand why we need std::move_if_noexcept in 
>unordered containers or even rb_tree. Couldn't we just use std::move ? 

No. If moving can throw then we can't provide strong exception safety.

>I don't understand what we are trying to avoid with this noexcept 
>check.

Then maybe stop trying to change how it works :-)
diff mbox series

Patch

diff --git a/libstdc++-v3/include/bits/stl_pair.h b/libstdc++-v3/include/bits/stl_pair.h
index 48af2b02ef9..85aad838860 100644
--- a/libstdc++-v3/include/bits/stl_pair.h
+++ b/libstdc++-v3/include/bits/stl_pair.h
@@ -528,6 +528,14 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       typedef pair<__ds_type1, __ds_type2> 	      __pair_type;
       return __pair_type(std::forward<_T1>(__x), std::forward<_T2>(__y));
     }
+
+  template<typename _T1, typename _T2>
+    struct __move_if_noexcept_cond<pair<_T1, _T2>>
+    : public __and_<is_copy_constructible<pair<_T1, _T2>>,
+		    __not_<__and_<
+	       __or_<is_const<_T1>, is_nothrow_move_constructible<_T1>>,
+	       __or_<is_const<_T2>, is_nothrow_move_constructible<_T2>>>>>::type
+  { };
 #else
   template<typename _T1, typename _T2>
     inline pair<_T1, _T2>
diff --git a/libstdc++-v3/testsuite/20_util/move_if_noexcept/1.cc b/libstdc++-v3/testsuite/20_util/move_if_noexcept/1.cc
index 078ccb83d36..b6f01097e40 100644
--- a/libstdc++-v3/testsuite/20_util/move_if_noexcept/1.cc
+++ b/libstdc++-v3/testsuite/20_util/move_if_noexcept/1.cc
@@ -33,7 +33,7 @@  struct noexcept_move_copy
 
   noexcept_move_copy(const noexcept_move_copy&) = default;
 
-  operator bool() { return status; }
+  operator bool() const { return status; }
 
 private:
   bool status;
@@ -50,7 +50,7 @@  struct noexcept_move_no_copy
 
   noexcept_move_no_copy(const noexcept_move_no_copy&) = delete;
 
-  operator bool() { return status; }
+  operator bool() const { return status; }
 
 private:
   bool status;
@@ -67,7 +67,7 @@  struct except_move_copy
 
   except_move_copy(const except_move_copy&) = default;
 
-  operator bool() { return status; }
+  operator bool() const { return status; }
 
 private:
   bool status;
@@ -84,7 +84,7 @@  struct except_move_no_copy
 
   except_move_no_copy(const except_move_no_copy&) = delete;
 
-  operator bool() { return status; }
+  operator bool() const { return status; }
 
 private:
   bool status;
@@ -110,8 +110,38 @@  test01()
   VERIFY( emnc1 == false );
 }
 
+void
+test02()
+{
+  std::pair<noexcept_move_copy, noexcept_move_copy> nemc1;
+  auto nemc2 __attribute__((unused)) = std::move_if_noexcept(nemc1);
+  VERIFY( nemc1.first == false );
+  VERIFY( nemc1.second == false );
+
+  std::pair<except_move_copy, noexcept_move_copy> emc1;
+  auto emc2 __attribute__((unused)) = std::move_if_noexcept(emc1);
+  VERIFY( emc1.first == true );
+  VERIFY( emc1.second == true );
+
+  std::pair<except_move_no_copy, noexcept_move_copy> emnc1;
+  auto emnc2 __attribute__((unused)) = std::move_if_noexcept(emnc1);
+  VERIFY( emnc1.first == false );
+  VERIFY( emnc1.second == false );
+
+  std::pair<const except_move_copy, noexcept_move_copy> cemc1;
+  auto cemc2 __attribute__((unused)) = std::move_if_noexcept(cemc1);
+  VERIFY( cemc1.first == true );
+  VERIFY( cemc1.second == false );
+
+  std::pair<noexcept_move_no_copy, noexcept_move_copy> nemnc1;
+  auto nemnc2 __attribute__((unused)) = std::move_if_noexcept(nemnc1);
+  VERIFY( nemnc1.first == false );
+  VERIFY( nemnc1.second == false );
+}
+
 int main()
 {
   test01();
+  test02();
   return 0;
 }
diff --git a/libstdc++-v3/testsuite/23_containers/unordered_map/allocator/move_assign.cc b/libstdc++-v3/testsuite/23_containers/unordered_map/allocator/move_assign.cc
index b27269e607a..d1be3adaae5 100644
--- a/libstdc++-v3/testsuite/23_containers/unordered_map/allocator/move_assign.cc
+++ b/libstdc++-v3/testsuite/23_containers/unordered_map/allocator/move_assign.cc
@@ -21,6 +21,7 @@ 
 #include <testsuite_hooks.h>
 #include <testsuite_allocator.h>
 #include <testsuite_counter_type.h>
+#include <testsuite_rvalref.h>
 
 using __gnu_test::propagating_allocator;
 using __gnu_test::counter_type;
@@ -49,8 +50,10 @@  void test01()
   VERIFY( 1 == v1.get_allocator().get_personality() );
   VERIFY( 2 == v2.get_allocator().get_personality() );
 
-  // No move because key is const.
-  VERIFY( counter_type::move_assign_count == 0  );
+  // Key copied, value moved.
+  VERIFY( counter_type::copy_count == 1  );
+  VERIFY( counter_type::move_count == 1  );
+  VERIFY( counter_type::destructor_count == 4 );
 }
 
 void test02()
@@ -79,15 +82,41 @@  void test02()
   VERIFY(0 == v1.get_allocator().get_personality());
   VERIFY(1 == v2.get_allocator().get_personality());
 
-  VERIFY( counter_type::move_assign_count == 0 );
+  VERIFY( counter_type::copy_count == 0  );
+  VERIFY( counter_type::move_count == 0  );
   VERIFY( counter_type::destructor_count == 2 );
 
   VERIFY( it == v2.begin() );
 }
 
+void test03()
+{
+  using __gnu_test::rvalstruct;
+
+  typedef std::pair<const int, rvalstruct> value_type;
+  typedef propagating_allocator<value_type, false> alloc_type;
+  typedef std::unordered_map<int, rvalstruct, std::hash<int>,
+			     std::equal_to<int>,
+			     alloc_type> test_type;
+
+  test_type v1(alloc_type(1));
+  v1.emplace(std::piecewise_construct,
+	     std::make_tuple(1), std::make_tuple(1));
+
+  test_type v2(alloc_type(2));
+  v2.emplace(std::piecewise_construct,
+	     std::make_tuple(2), std::make_tuple(2));
+
+  v2 = std::move(v1);
+
+  VERIFY( 1 == v1.get_allocator().get_personality() );
+  VERIFY( 2 == v2.get_allocator().get_personality() );
+}
+
 int main()
 {
   test01();
   test02();
+  test03();
   return 0;
 }