Re: [PATCH] PR77990 refactor unique_ptr to encapsulate tuple

2016-10-19 Thread Jonathan Wakely

On 17/10/16 14:37 +0100, Jonathan Wakely wrote:

We are incorrectly requiring unique_ptr deleters to be copyable here:

explicit
unique_ptr(pointer __p) noexcept
: _M_t(__p, deleter_type())
{ }

We could just do:

explicit
unique_ptr(pointer __p) noexcept
: _M_t()
{ std::get<0>(_M_t) = __p; }

But having to deal directly with the std::tuple inside unique_ptr has
been bothering me for some time. The tuple is used so we get the empty
base-class optimisation for the deleter, but that implementation
detail

This patch refactors unique_ptr to put the std::tuple member into a
new type which provides named accessors for the tuple elements, so we
can stop using get<0> and get<1>. That new type can also provide a
single-argument constructor to fix the copyable requirement for
deleters. This also removes the code for deducing the pointer type
which is duplciated in unique_ptr and unique_ptr, and while in
the neighbourhood I changed it from old-school SFINAE using overloaded
functions to the new hotness with __void_t<>.

I intend to commit this to trunk, but on the branches I'll just fix
the constructor as shown above, as it's a smaller change.


PR libstdc++/77990
* include/bits/unique_ptr.h (__uniq_ptr_impl): New type to
encapsulate implementation details.
(unique_ptr::unique_ptr(_Up)): Don't copy deleter object.
(unique_ptr::get, unique_ptr::get_deleter, unique_ptr::release):
Call member functions of implementation object.
(unique_ptr): Likewise.
* python/libstdcxx/v6/printers.py (UniquePointerPrinter): Adjust for
new implementation.
* python/libstdcxx/v6/xmethods.py (UniquePtrGetWorker): Likewise.
* testsuite/20_util/unique_ptr/assign/48635_neg.cc: Adjust dg-error
lines.
* testsuite/20_util/unique_ptr/assign/cv_qual.cc: Likewise.
* testsuite/20_util/unique_ptr/cons/cv_qual.cc: Likewise.
* testsuite/20_util/unique_ptr/cons/77990.cc: New test.

Tested powerpc64le-linux.


Committed to trunk. Here's the smaller patch I'm committing to the
branches.


commit 81ea53cd80fe7aaa3a323dba58bdb2259d672be3
Author: Jonathan Wakely 
Date:   Wed Oct 19 10:21:58 2016 +0100

PR77990 fix unique_ptr for non-copyable deleters

	PR libstdc++/77990
	* include/bits/unique_ptr.h (unique_ptr::unique_ptr(pointer)): Set
	pointer member after value-initialization of tuple.
	* testsuite/20_util/unique_ptr/assign/48635_neg.cc: Adjust dg-errors.
	* testsuite/20_util/unique_ptr/cons/77990.cc: New test.
	* testsuite/20_util/unique_ptr/cons/cv_qual.cc: Adjust dg-error.

diff --git a/libstdc++-v3/include/bits/unique_ptr.h b/libstdc++-v3/include/bits/unique_ptr.h
index c4a6d2f..8e30287 100644
--- a/libstdc++-v3/include/bits/unique_ptr.h
+++ b/libstdc++-v3/include/bits/unique_ptr.h
@@ -168,9 +168,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
*/
   explicit
   unique_ptr(pointer __p) noexcept
-  : _M_t(__p, deleter_type())
-  { static_assert(!is_pointer::value,
-		 "constructed with null function pointer deleter"); }
+  : _M_t()
+  {
+	std::get<0>(_M_t) = __p;
+	static_assert(!is_pointer::value,
+		 "constructed with null function pointer deleter");
+  }
 
   /** Takes ownership of a pointer.
*
diff --git a/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc b/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
index 16ff8ae..785f9ad 100644
--- a/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
+++ b/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
@@ -43,10 +43,10 @@ void f()
   std::unique_ptr ud(nullptr, d);
   ub = std::move(ud); // { dg-error "no match" }
   ub2 = ud; // { dg-error "no match" }
-// { dg-error "no type" "" { target *-*-* } 269 }
+// { dg-error "no type" "" { target *-*-* } 272 }
 
   std::unique_ptr uba(nullptr, b);
   std::unique_ptr uda(nullptr, d);
   uba = std::move(uda); // { dg-error "no match" }
-// { dg-error "no type" "" { target *-*-* } 537 }
+// { dg-error "no type" "" { target *-*-* } 540 }
 }
diff --git a/libstdc++-v3/testsuite/20_util/unique_ptr/cons/77990.cc b/libstdc++-v3/testsuite/20_util/unique_ptr/cons/77990.cc
new file mode 100644
index 000..1acc313
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/unique_ptr/cons/77990.cc
@@ -0,0 +1,28 @@
+// Copyright (C) 2016 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR 

Re: [PATCH] PR77990 refactor unique_ptr to encapsulate tuple

2016-10-18 Thread Pedro Alves
On 10/18/2016 12:54 PM, Jonathan Wakely wrote:

> I'll wait a bit longer for any objections, as the refactoring could be
> seen as unnecessary churn, but I think it's valuable housekeeping.

Having stared at std::unique_ptr a lot recently, I like this, FWIW.

Thanks,
Pedro Alves



Re: [PATCH] PR77990 refactor unique_ptr to encapsulate tuple

2016-10-18 Thread Jonathan Wakely

On 17/10/16 14:37 +0100, Jonathan Wakely wrote:

We are incorrectly requiring unique_ptr deleters to be copyable here:

explicit
unique_ptr(pointer __p) noexcept
: _M_t(__p, deleter_type())
{ }

We could just do:

explicit
unique_ptr(pointer __p) noexcept
: _M_t()
{ std::get<0>(_M_t) = __p; }

But having to deal directly with the std::tuple inside unique_ptr has
been bothering me for some time. The tuple is used so we get the empty
base-class optimisation for the deleter, but that implementation
detail


ops, a dangling sentence. I meant to say that the tuple implementation
details leaks into the definition of lots of members, which have to
say std::get<0>(_M_t) or std::get<1>(_M_t) instead of using more
natural member names.


This patch refactors unique_ptr to put the std::tuple member into a
new type which provides named accessors for the tuple elements, so we
can stop using get<0> and get<1>. That new type can also provide a
single-argument constructor to fix the copyable requirement for
deleters. This also removes the code for deducing the pointer type
which is duplciated in unique_ptr and unique_ptr, and while in
the neighbourhood I changed it from old-school SFINAE using overloaded
functions to the new hotness with __void_t<>.

I intend to commit this to trunk, but on the branches I'll just fix
the constructor as shown above, as it's a smaller change.


I'll wait a bit longer for any objections, as the refactoring could be
seen as unnecessary churn, but I think it's valuable housekeeping.