User account creation filtered due to spam.

Bug 48635 - [C++0x] unique_ptr<T, D&> moves the deleter instead of copying it
Summary: [C++0x] unique_ptr<T, D&> moves the deleter instead of copying it
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.7.0
: P3 normal
Target Milestone: 4.6.1
Assignee: Paolo Carlini
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-15 20:59 UTC by Daniel Krügler
Modified: 2011-04-17 21:46 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2011-04-16 00:25:26


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Krügler 2011-04-15 20:59:08 UTC
The following program using 4.7.0 20110409 (experimental) should print "copy", but it prints "move" instead:

//-------------
#include <memory>
#include <utility>
#include <iostream>

struct Deleter {
  Deleter() = default;
  Deleter(const Deleter&) = default;
  Deleter(Deleter&&) = default;
  Deleter& operator=(const Deleter&) {
    std::cout << "copy" << std::endl;
    return *this;
  }
  Deleter& operator=(Deleter&&) {
    std::cout << "move" << std::endl;
    return *this;
  }
  template<class T>
  void operator()(T* p) const
  {
    delete p;
  }
};

int main() {
  Deleter d1, d2;
  std::unique_ptr<int, Deleter&> p1(new int, d1), p2(nullptr, d2);
  p2 = std::move(p1);
}
//-------------

The reason for the failure is, that the library implementation of the move-assignment operator of std::unique_ptr (and of it's templated variant) uses std::move to transfer the deleter from the source to the target, but as of [unique.ptr.single.asgn] p. 2 and p. 6 it shall use std::forward instead. Doing it correctly will ensure that the deleter is copied and not moved in this case, which is an intended design aim of std::unique_ptr with deleters that are lvalue-references.

The necessary fix is to replace the usage of std::move at the two places by std::forward<deleter_type>.
Comment 1 Daniel Krügler 2011-04-15 21:44:56 UTC
(In reply to comment #0)

The exactly same problem exists for the specialization std::unique_ptr<T[], D&>.
Comment 2 Paolo Carlini 2011-04-16 00:25:26 UTC
Thanks, Daniel. I'll fix this momentarily.
Comment 3 paolo@gcc.gnu.org 2011-04-16 00:55:49 UTC
Author: paolo
Date: Sat Apr 16 00:55:43 2011
New Revision: 172532

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=172532
Log:
2011-04-15  Daniel Krugler  <daniel.kruegler@googlemail.com>
	    Paolo Carlini  <paolo.carlini@oracle.com>

	PR libstdc++/48635
	* include/bits/unique_ptr.h (unique_ptr<>::operator=(unique_ptr&&),
	unique_ptr<>::operator=(unique_ptr<>&&),
	unique_ptr<_Tp[],>::operator=(unique_ptr&&),
	unique_ptr<_Tp[],>::operator=(unique_ptr<>&&)): Forward the deleter
	instead of moving it.
	* testsuite/20_util/unique_ptr/assign/48635.cc: New.

Added:
    trunk/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/unique_ptr.h
Comment 4 paolo@gcc.gnu.org 2011-04-16 00:55:59 UTC
Author: paolo
Date: Sat Apr 16 00:55:53 2011
New Revision: 172533

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=172533
Log:
2011-04-15  Daniel Krugler  <daniel.kruegler@googlemail.com>
	    Paolo Carlini  <paolo.carlini@oracle.com>

	PR libstdc++/48635
	* include/bits/unique_ptr.h (unique_ptr<>::operator=(unique_ptr&&),
	unique_ptr<>::operator=(unique_ptr<>&&),
	unique_ptr<_Tp[],>::operator=(unique_ptr&&),
	unique_ptr<_Tp[],>::operator=(unique_ptr<>&&)): Forward the deleter
	instead of moving it.
	* testsuite/20_util/unique_ptr/assign/48635.cc: New.

Added:
    branches/gcc-4_6-branch/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635.cc
Modified:
    branches/gcc-4_6-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_6-branch/libstdc++-v3/include/bits/unique_ptr.h
Comment 5 Paolo Carlini 2011-04-16 00:56:57 UTC
Done. Fixed mainline and 4_6-branch.
Comment 6 Daniel Krügler 2011-04-17 19:18:01 UTC
(In reply to comment #5)
> Done. Fixed mainline and 4_6-branch.

Looks good, thanks, but I just recognize that there is a library issue in regard to the template version. The FDIS correctly applied the corresponding proposal, but for some reason that is unclear, [unique.ptr.single.asgn] p. 6 describes the effects as "std::forward<D>(u.get_deleter())". I verified with discussion with Howard that this should have been "std::forward<E>(u.get_deleter())" instead, as it was suggested in

http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#983
Comment 7 Paolo Carlini 2011-04-17 19:44:51 UTC
Ok... Do we have testcases for that?

By the way, I noticed that in the templated *constructor* we have been using D instead of E in the forward, and that doesn't seem correct vs the FDIS itself. What do you think? The below regtests fine:

Index: include/bits/unique_ptr.h
===================================================================
--- include/bits/unique_ptr.h	(revision 172616)
+++ include/bits/unique_ptr.h	(working copy)
@@ -153,7 +153,7 @@
 		   && std::is_convertible<_Ep, _Dp>::value))>
 	     ::type>
 	unique_ptr(unique_ptr<_Up, _Ep>&& __u)
-	: _M_t(__u.release(), std::forward<deleter_type>(__u.get_deleter()))
+	: _M_t(__u.release(), std::forward<_Ep>(__u.get_deleter()))
 	{ }
 
 #if _GLIBCXX_USE_DEPRECATED
@@ -186,7 +186,7 @@
 	operator=(unique_ptr<_Up, _Ep>&& __u)
 	{
 	  reset(__u.release());
-	  get_deleter() = std::forward<deleter_type>(__u.get_deleter());
+	  get_deleter() = std::forward<_Ep>(__u.get_deleter());
 	  return *this;
 	}
 
@@ -306,7 +306,7 @@
 
       template<typename _Up, typename _Ep>
 	unique_ptr(unique_ptr<_Up, _Ep>&& __u)
-	: _M_t(__u.release(), std::forward<deleter_type>(__u.get_deleter()))
+	: _M_t(__u.release(), std::forward<_Ep>(__u.get_deleter()))
 	{ }
 
       // Destructor.
@@ -326,7 +326,7 @@
 	operator=(unique_ptr<_Up, _Ep>&& __u)
 	{
 	  reset(__u.release());
-	  get_deleter() = std::forward<deleter_type>(__u.get_deleter());
+	  get_deleter() = std::forward<_Ep>(__u.get_deleter());
 	  return *this;
 	}
Comment 8 Daniel Krügler 2011-04-17 20:12:09 UTC
(In reply to comment #7)
> Ok... Do we have testcases for that?

I have two test cases for the *assignment* situation, I invented them out of my head and I hope the code below does not too many typos and thinkos:

1) This program should be well-formed according to FDIS wording, but ill-formed according to the intended wording:

#include <memory>
#include <utility>

struct D;

struct B {
 B& operator=(D&) = delete;
 template<class T>
   void operator()(T*) const {}
};

struct D : B {};

int main()
{
 B b;
 D d;
 std::unique_ptr<void, B&> ub(nullptr, b);
 std::unique_ptr<void, D&> ud(nullptr, d);
 ub = std::move(ud); // Should be rejected
}

2) The same scenario here:

struct D2;

struct B2 {
 B2& operator=(D2&) = delete;
 template<class T>
   void operator()(T*) const {}
};

struct D2 {
 B2 b;
 operator B2&() { return b; }
 template<class T>
   void operator()(T*) const {}
};

int main()
{
 B2 b;
 D2 d;
 std::unique_ptr<void, B2&> ub(nullptr, b);
 std::unique_ptr<void, D2&> ud(nullptr, d);
 ub = std::move(ud); // Should be rejected
}

> By the way, I noticed that in the templated *constructor* we have been using D
> instead of E in the forward, and that doesn't seem correct vs the FDIS itself.
> What do you think? The below regtests fine:

I agree that the suggested fixes are necessary and they look correct to me.
Comment 9 paolo@gcc.gnu.org 2011-04-17 21:46:15 UTC
Author: paolo
Date: Sun Apr 17 21:46:11 2011
New Revision: 172619

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=172619
Log:
2011-04-17  Daniel Krugler  <daniel.kruegler@googlemail.com>
	    Paolo Carlini  <paolo.carlini@oracle.com>

	PR libstdc++/48635 (again)
	* include/bits/unique_ptr.h (unique_ptr<>::unique_ptr(unique_ptr<>&&),
	unique_ptr<_Tp[]>::unique_ptr(unique_ptr<>&&),
	unique_ptr<>::operator=(unique_ptr<>&&),
	unique_ptr<_Tp[]>::operator=(unique_ptr<>&&)): Use forward<_Ep>, not
	forward<_Dp>, to forward the deleter.
	* testsuite/20_util/unique_ptr/assign/48635_neg.cc: New.

Added:
    trunk/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/unique_ptr.h
Comment 10 paolo@gcc.gnu.org 2011-04-17 21:46:23 UTC
Author: paolo
Date: Sun Apr 17 21:46:20 2011
New Revision: 172620

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=172620
Log:
2011-04-17  Daniel Krugler  <daniel.kruegler@googlemail.com>
	    Paolo Carlini  <paolo.carlini@oracle.com>

	PR libstdc++/48635 (again)
	* include/bits/unique_ptr.h (unique_ptr<>::unique_ptr(unique_ptr<>&&),
	unique_ptr<_Tp[]>::unique_ptr(unique_ptr<>&&),
	unique_ptr<>::operator=(unique_ptr<>&&),
	unique_ptr<_Tp[]>::operator=(unique_ptr<>&&)): Use forward<_Ep>, not
	forward<_Dp>, to forward the deleter.
	* testsuite/20_util/unique_ptr/assign/48635_neg.cc: New.

Added:
    branches/gcc-4_6-branch/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
Modified:
    branches/gcc-4_6-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_6-branch/libstdc++-v3/include/bits/unique_ptr.h