[Bug libstdc++/48635] New: [C++0x] unique_ptr<T, D&> moves the deleter instead of copying it

[Bug libstdc++/48635] New: [C++0x] unique_ptr<T, D&> moves the deleter instead of copying it

[daniel.kruegler at googlemail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48635

           Summary: [C++0x] unique_ptr<T, D&> moves the deleter instead of
                    copying it
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: daniel.kruegler@googlemail.com


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>.

[Bug libstdc++/48635] [C++0x] unique_ptr<T, D&> moves the deleter instead of copying it

[daniel.kruegler at googlemail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48635

--- Comment #1 from Daniel Krügler <daniel.kruegler at googlemail dot com> 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&>.

[Bug libstdc++/48635] [C++0x] unique_ptr<T, D&> moves the deleter instead of copying it

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48635

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011.04.16 00:25:26
         AssignedTo|unassigned at gcc dot       |paolo.carlini at oracle dot
                   |gnu.org                     |com
   Target Milestone|---                         |4.6.1
     Ever Confirmed|0                           |1

--- Comment #2 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-04-16 00:25:26 UTC ---
Thanks, Daniel. I'll fix this momentarily.

[Bug libstdc++/48635] [C++0x] unique_ptr<T, D&> moves the deleter instead of copying it

[paolo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48635

--- Comment #3 from paolo at gcc dot gnu.org <paolo at gcc dot 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

[Bug libstdc++/48635] [C++0x] unique_ptr<T, D&> moves the deleter instead of copying it

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48635

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #5 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-04-16 00:56:57 UTC ---
Done. Fixed mainline and 4_6-branch.

[Bug libstdc++/48635] [C++0x] unique_ptr<T, D&> moves the deleter instead of copying it

[paolo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48635

--- Comment #4 from paolo at gcc dot gnu.org <paolo at gcc dot 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

[Bug libstdc++/48635] [C++0x] unique_ptr<T, D&> moves the deleter instead of copying it

[daniel.kruegler at googlemail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48635

--- Comment #6 from Daniel Krügler <daniel.kruegler at googlemail dot com> 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

[Bug libstdc++/48635] [C++0x] unique_ptr<T, D&> moves the deleter instead of copying it

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48635

--- Comment #7 from Paolo Carlini <paolo.carlini at oracle dot com> 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;
     }

[Bug libstdc++/48635] [C++0x] unique_ptr<T, D&> moves the deleter instead of copying it

[daniel.kruegler at googlemail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48635

--- Comment #8 from Daniel Krügler <daniel.kruegler at googlemail dot com> 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.

[Bug libstdc++/48635] [C++0x] unique_ptr<T, D&> moves the deleter instead of copying it

[paolo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48635

--- Comment #9 from paolo at gcc dot gnu.org <paolo at gcc dot 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

[Bug libstdc++/48635] [C++0x] unique_ptr<T, D&> moves the deleter instead of copying it

[paolo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48635

--- Comment #10 from paolo at gcc dot gnu.org <paolo at gcc dot 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