[move][unique_ptr] c++14 unique_ptr comes to town
Hi to all, We've discussed about a boostified version of std::unique_ptr several times in later years. I decided we could not continue with this hole in the Boost toolset and implemented in Boost.Move a unique_ptr that follows the C++14 interface. Commit here: SHA-1: e1da7c5ca1162841f643331af891a9a730eba118 http://github.com/boostorg/move/commit/e1da7c5ca1162841f643331af891a9a730eba... This implementation is also usable from C++03, and apart from the general limitations of the Boost.Move machinery, it should be really uesful for C++03 users. I would like to make this implementation the official boost::unique_ptr implementation, but until the community approves this, it's in boost::movelib namespace (boost::move it's a function so I needed to tweak the namespace name). I would like to thank Howard Hinnant for his great public implementation of the emulated unique_ptr published here: http://home.roadrunner.com/~hinnant/unique_ptr03.html I've ported and refactored Howard's extensive testsuite to implement boost test cases. Really useful to detect many implementation errors and details. I've found unique_ptr much harder to write than I expected. The standard text is really detailed and helps implementors, but things like allowed conversion between pointers and deleters and several ICEs and different SFINAE parties on many compilers made the work quite entertaining. The API is taken from the draft n3797 and the implementation has some differences with the standard. The main reason for this is that I wanted to make unique_ptr as lightweight as possible avoiding external dependencies, as it's interesting to implement idioms like PIMPL. Main attributes and differences with the standard: -> Implemented in two headers: <boost/move/unique_ptr.hpp> and <boost/move/make unique.hpp>. -> <boost/move/unique_ptr.hpp>: Boost.Move is the only heavyweight dependency. Not even that, as unique_ptr.hpp only uses boost/move/utility_core.hpp, that does not depend on any external library excepct BoostConfig and StaticAssert. No TypeTraits, MPL and others. Only Boost.Config, BOOST_STATIC_ASSERT and BOOST_ASSERT are used in the implementation. No standard library headers used (no pointer_traits, memory or other utilities). -> <boost/move/make unique.hpp>: Boost.PP is used in compilers without variadic templates. This is the main reason to have a separate make_unique.hpp header and avoid pulling Boost.PP for c++03 users that only want to have C++11 unique_ptr. -> unique_ptr's "operator <" is implemented as x.get() < y.get() instead of std::less<CT>(). std::less requires <funcional>. std::common_type requires C++11 standard library or some partial emulation based on Boost.TypeOf. I don't think 99,99% of users will notice the difference. For raw pointers, in all our platforms comparisons provide strict weak ordering. If a smart pointer writer specializes std::less for its smart pointer, it should be a law against that. -> It's assignable from literal 0. I found some problems in some compilers trying to write a constructor for that. -> nullptr support is conditional on !defined(BOOST_NO_CXX11_NULLPTR) but I think really should have a Boost nullptr for C++03 compilers. Let's put Meyer's nullptr (http://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/nullptr) in Boost for BOOST_NO_CXX11_NULLPTR compilers. -> unique_ptr<T[]> is convertible to other types when conversion is considered safe. This means that unique_ptr<T[]> is convertible to unique_ptr<const volatile T[]>. Conversion from T[] to U[] is considered safe if remove_cv<T> is the same type as remove_cv<U> and unique_ptr<T[]>::pointer is convertible to unique_ptr<U[]>::pointer. This is a feature that might go to C++17 (http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#2118). Safe conversions in reset() and constructors from pointers are also implemented in unique_ptr<T[]>. I tested the implementation in GCC{4.4, 4.6, 4.7, 4.8} Clang3.4 and MSVC{7.1, 8.0, 9.0, 10.0, 11.0, 12.0} in 32/64 bits, debug/release and C++03/C++11 modes (where applicable). The class documentation is generated with Boost.Move documentation and the most part of it is taken from the standard. I haven't added a tutorial to Boost.Move but I plan to it based on Howard's c++03 unique_ptr tutorial. I hope you find this addition to Boost.Move useful. If you think the implementation is good enough to be the official boost::unique_ptr implementation living in Boost.SmartPtr, porting should be easy, except for the documentation part. That would require either combining somehow handwritten and Quickbook produced documentation or writing new docs for unique_ptr. Best, Ion
Hi Ion, Ion Gaztañaga wrote:
Hi to all,
We've discussed about a boostified version of std::unique_ptr several times in later years. I decided we could not continue with this hole in the Boost toolset and implemented in Boost.Move a unique_ptr that follows the C++14 interface. That's great news. Boost should have its own implementation of unique_ptr. Thanks for your hard work!
<snip>
-> Implemented in two headers: <boost/move/unique_ptr.hpp> and <boost/move/make unique.hpp>.
-> <boost/move/unique_ptr.hpp>: Boost.Move is the only heavyweight dependency. Not even that, as unique_ptr.hpp only uses boost/move/utility_core.hpp, that does not depend on any external library excepct BoostConfig and StaticAssert. No TypeTraits, MPL and others. Only Boost.Config, BOOST_STATIC_ASSERT and BOOST_ASSERT are used in the implementation. No standard library headers used (no pointer_traits, memory or other utilities).
-> <boost/move/make unique.hpp>: Boost.PP is used in compilers without variadic templates. This is the main reason to have a separate make_unique.hpp header and avoid pulling Boost.PP for c++03 users that only want to have C++11 unique_ptr.
AFAIS this is in line with the Boost.SmartPtr library, e.g. shared_ptr and make_shared aren't included together, correct me if I'm wrong. The difference is that make_shared doesn't emulate variadic templates using Boost.PP. And there is an implementation of boost::make_unique returning std::unique_ptr<> so I'm guessing that there may be a problem with backward compatibility. <snip>
I hope you find this addition to Boost.Move useful. If you think the implementation is good enough to be the official boost::unique_ptr implementation living in Boost.SmartPtr, porting should be easy, except for the documentation part. That would require either combining somehow handwritten and Quickbook produced documentation or writing new docs for unique_ptr.
IMHO we should go towards the official implementation. However AFAIU unique_ptr might not reside in the Boost.SmartPtr, it might stay in Boost.Move module or be moved to some separate one (UniquePtr). It's because it'd introduce additional dependencies to SmartPtr (Move and PP). It should be transparent for the user. The only problem I see is the boost/make_unique.hpp (in SmartPtr) including the implementation working differently than yours. Regards, Adam
El 23/08/2014 23:47, Adam Wulkiewicz escribió:
AFAIS this is in line with the Boost.SmartPtr library, e.g. shared_ptr and make_shared aren't included together, correct me if I'm wrong. The difference is that make_shared doesn't emulate variadic templates using Boost.PP. And there is an implementation of boost::make_unique returning std::unique_ptr<> so I'm guessing that there may be a problem with backward compatibility.
<nod> I didn't notice that. An alternative to maintain backwards compatibility is to change header and function names. But I noticed those headers are from January 2014, released only in Boost 1.56. I doubt the change will break much user code. If backwards compatibility is not required we could somehow try to inform users that they should not use it and replace it in Boost 1.57. That's something we should decide ASAP. And release Boost 1.57 ASAP too ;-) In any case I think some functions from current make_unique.hpp, like "make_unique_noinit", can be useful, specially when allocating big memory buffers of POD types users don't need to value initialize. Best, Ion
Adam Wulkiewicz wrote:
And there is an implementation of boost::make_unique returning std::unique_ptr<> so I'm guessing that there may be a problem with backward compatibility.
Releasing that (in 1.56) was perhaps a mistake. boost::make_unique should certainly return boost::unique_ptr.
On 22/08/2014 02:42 p.m., Ion Gaztañaga wrote:
I hope you find this addition to Boost.Move useful. If you think the implementation is good enough to be the official boost::unique_ptr implementation living in Boost.SmartPtr, porting should be easy, except for the documentation part. That would require either combining somehow handwritten and Quickbook produced documentation or writing new docs for unique_ptr.
Thank you for your efforts! I really hope `boost:unique_ptr` ends up a member of Boost.SmartPtr. As per the documentation issue, I can volunteer to port it to Quickbook (unless someone else wants to). Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
Ion Gaztañaga wrote:
I would like to make this implementation the official boost::unique_ptr implementation, but until the community approves this, it's in boost::movelib namespace (boost::move it's a function so I needed to tweak the namespace name).
I actually have my own prototype implementation (attached) that I used to test my ideas of how unique_ptr<T[]> and unique_ptr<T[N]> should behave with respect to conversions between them (modeled after shared_ptr.) It's however a prototype; meaning that it relies on C++11 features, does not optimize out empty deleters, has no tests. I used the libc++ test suite to test it, but that's not suitable for Boost. It didn't occur to me that Howard has already written the test suite in a more suitable form. :-) I intended to eventually bring this up to production quality and make it boost::unique_ptr, but couldn't spare the time.
unique_ptr's "operator <" is implemented as x.get() < y.get() instead of std::less<CT>().
That exactly what I did too. :-)
El 24/08/2014 18:31, Peter Dimov escribió:
Ion Gaztañaga wrote:
I would like to make this implementation the official boost::unique_ptr implementation, but until the community approves this, it's in boost::movelib namespace (boost::move it's a function so I needed to tweak the namespace name).
I actually have my own prototype implementation (attached) that I used to test my ideas of how unique_ptr<T[]> and unique_ptr<T[N]> should behave with respect to conversions between them (modeled after shared_ptr.)
Nice. You don't specialize the class, but just disallow some operations depending on the single/array instantiation. It's much more compact ;-) The boost::move implementation might be simplified with this approach.
I intended to eventually bring this up to production quality and make it boost::unique_ptr, but couldn't spare the time.
Would you agree making boost::movelib::unique_ptr boost::unique_ptr? I can think about some gradual steps. Fast steps: 1) Change the namespace from boost::movelib:: to boost:: 2) Create a boost/unique_ptr.hpp header redirecting to boost/move/unique_ptr.hpp 3) Replace boost/make_unique.hpp redirecting to boost/move/make_unique (this is breaking and would need consensus). Slow steps (users should not notice any problem in the transition) 4) Decide if the implementation should live in boost/move or boost/smart_ptr. 5) If we decide to move it to Boost.SmartPtr, start writing the docs, pass the implementation and tests to SmartPtr test cases, etc. Sounds ok? If so, I can prepare some pull requests to develop. Best, Ion
On Mon, Aug 25, 2014 at 3:36 AM, Ion Gaztañaga wrote:
Would you agree making boost::movelib::unique_ptr boost::unique_ptr? I can think about some gradual steps.
I like the idea of a boost::unique_ptr in Boost.SmartPtr, but my initial desire was for it to start as a perfect implementation of the std::unique_ptr specification in the standard, followed by modifications that reconcile it with any interface changes proposed in boost::shared_ptr. I also assumed compiler support for the necessary C++11 language features to support that implementation: i.e. while I see value in supporting C++03 via Boost.Move, I don't know if there is such a desire for a boost::unique_ptr C++03 emulation. It looks like Peter has already started on an implementation, and I like the approach he took. If he has the time, and the inclination, I would like to have him drive that to completion to as the Boost.SmartPtr unique_ptr implementation. What would be nicer still, is if Boost.Move could then leverage that implementation to provide a C++03 emulation in boost::movelib or boost::moveable etc. but that might be a taller order. Glen
El 25/08/2014 20:16, Glen Fernandes escribió:
On Mon, Aug 25, 2014 at 3:36 AM, Ion Gaztañaga wrote:
Would you agree making boost::movelib::unique_ptr boost::unique_ptr? I can think about some gradual steps.
I like the idea of a boost::unique_ptr in Boost.SmartPtr, but my initial desire was for it to start as a perfect implementation of the std::unique_ptr specification in the standard, followed by modifications that reconcile it with any interface changes proposed in boost::shared_ptr. I also assumed compiler support for the necessary C++11 language features to support that implementation: i.e. while I see value in supporting C++03 via Boost.Move, I don't know if there is such a desire for a boost::unique_ptr C++03 emulation.
I think there is desire such a desire, as expressed several times in the mailing list. That does not mean that we can't have one implementation for c++03 and another one for C++11, if that eases maintenance. The C++11 implementation could avoid depending on Boost.Move, but in any case, I think you should measure the impact of the Boost.Move dependency, as I think it's more lightweight (in preprocessed code size) than including <utility>. Best, Ion
On Mon, Aug 25, 2014 at 3:36 AM, Ion Gaztañaga wrote:
Would you agree making boost::movelib::unique_ptr boost::unique_ptr? I can think about some gradual steps. I like the idea of a boost::unique_ptr in Boost.SmartPtr, but my initial desire was for it to start as a perfect implementation of the std::unique_ptr specification in the standard, followed by modifications that reconcile it with any interface changes proposed in boost::shared_ptr. I also assumed compiler support for the necessary C++11 language features to support that implementation: i.e. while I see value in supporting C++03 via Boost.Move, I don't know if there is such a desire for a boost::unique_ptr C++03 emulation. I need a portable implementation in order to make the libraries I
Le 25/08/14 20:16, Glen Fernandes a écrit : maintain portable. Currently I'm using boost::interprocess::unique_ptr (which uses Boost.Move, but is not completely compatible to the C++11 specification), but I would prefer a portable implementation in SmartPtr as boost::unique_ptr.
It looks like Peter has already started on an implementation, and I like the approach he took. If he has the time, and the inclination, I would like to have him drive that to completion to as the Boost.SmartPtr unique_ptr implementation. What would be nicer still, is if Boost.Move could then leverage that implementation to provide a C++03 emulation in boost::movelib or boost::moveable etc. but that might be a taller order.
While I'm not against for C++11 only libraries, I think that this one must be provided for C++98 compilers. Best, Vicente
Vicente J. Botet Escriba wrote:
Le 25/08/14 20:16, Glen Fernandes a écrit :
It looks like Peter has already started on an implementation, and I like the approach he took. If he has the time, and the inclination, I would like to have him drive that to completion to as the Boost.SmartPtr unique_ptr implementation. What would be nicer still, is if Boost.Move could then leverage that implementation to provide a C++03 emulation in boost::movelib or boost::moveable etc. but that might be a taller order.
While I'm not against for C++11 only libraries, I think that this one must be provided for C++98 compilers.
I fully concur with the above statement. Why do you think Peter's implementation is better? AFAIU it's a prototype. Is it that good that you think it would be better to use it as a base? It certainly must be more clear/elegant since it relies on C++11 features. I'm guessing that after adding the move emulation for C++98, optimizations, workarounds, etc. the code would look different. Close to Ion's maybe? Without checking out the code IMHO more reasonable approach would be to take more complete implementation and then port some nice solutions from the other one but of course I may be missing something. Regards, Adam
Adam Wulkiewicz wrote:
Without checking out the code...
You should. It's not much.
IMHO more reasonable approach would be to take more complete implementation and then port some nice solutions from the other one but of course I may be missing something.
Ion's implementation is more complete in that it supports C++03 and optimizes out empty deleters. Mine is "more complete" in that it supports unique_ptr<T[N]> with proper shared_ptr-style conversions and is "better" in that it does all that with a single template, without triplication. I've also decided to make unique_ptr<T>( new U ) and unique_ptr<T>( unique_ptr<U>() ) only compile when U* can be deleted properly via T*, that is, when they share the same cv-less type or when T has a virtual destructor.
I'm guessing that after adding the move emulation for C++98, optimizations, workarounds, etc. the code would look different.
It certainly will look different. I'm still undecided on whether it'd be better to maintain two separate implementations, one C++11 and one C++03, or to #ifdef everything in a single file. Both approaches have their pros and cons. More head-scratching is required.
El 26/08/2014 18:54, Peter Dimov wrote:
Mine is "more complete" in that it supports unique_ptr<T[N]> with proper shared_ptr-style conversions and is "better" in that it does all that with a single template, without triplication.
Hi Peter, I've committed changeds to boost::movelib::unique_ptr now supports unique_ptr<T[N]>, adding the operator[] check mentioned in shared_ptr documentation. I have some doubts with these conversions. Currently I've implemented the conversions: 1) unique_ptr<T[]> -> unique_ptr<U[]> 2) unique_ptr<T[N]> -> unique_ptr<U[N]> 3) unique_ptr<T[N]> -> unique_ptr<U[]> if remove_cv_t<T[]> == remove_cv_t<U[]>, pointers are convertible and deleters are convertible (and this is the key point). Although not provided by the language (T(*)[N] is not convertible to U(*)[]), the third conversion is safe with the default deleter. A problem might occure if a user-provided deleter<T[N]> is be convertible to deleter<U[]> but it does not properly handle unknown-length arrays. The conversion will be unsafe. Deleter writers should be warned about this conversion. On the other hand, with the default_deleter it could be safe to convert (maybe if N >= M): 4) unique_ptr<T[N]> -> unique_ptr<U[M]> that is, we can safely create a slice of the array. I don't know if this conversion has any value. I understand conversion 3 is useful to get some data erasure and avoid propagating size though all the code. Is interesting to allow such conversion? Best, Ion
Ion Gaztañaga wrote:
On the other hand, with the default_deleter it could be safe to convert (maybe if N >= M):
4) unique_ptr<T[N]> -> unique_ptr<U[M]>
I don't support this conversion in shared_ptr and have no plans to. reinterpret_pointer_cast should work though.
A problem might occure if a user-provided deleter<T[N]> is be convertible to deleter<U[]> but it does not properly handle unknown-length arrays.
In general, if deleter<X> is convertible to deleter<Y> but does not handle Y properly, it might cause problems. So I'm not sure that this case is extraordinary in this sense. It's true that these cases are a bit convoluted because, esp. in the presence of a user-defined pointer type, we need to decide how much to trust the deleter and how much to override whatever it says WRT conversions. For instance, let's say unique_ptr<X1,D1> wants to convert to unique_ptr<X2,D2> and D1::pointer is convertible to D2::pointer and D1 is convertible to D2 _but_ X2 does not have a virtual destructor. Do we disallow the conversion as we do in the ordinary X1* -> X2* case when using the default deleter, or do we go ahead? The array case is similar, unique_ptr<X[],D1> -> unique_ptr<Y[],D2>, X(*)[] is not convertible to Y(*)[] but both D1::pointer and D1 are convertible to D2::pointer and D2. Legal or not?
El 02/09/2014 19:22, Peter Dimov escribió:
Ion Gaztañaga wrote:
On the other hand, with the default_deleter it could be safe to convert (maybe if N >= M):
4) unique_ptr<T[N]> -> unique_ptr<U[M]>
I don't support this conversion in shared_ptr and have no plans to. reinterpret_pointer_cast should work though.
Ok, I prototyped it and it worked, but found it too forced. You can always release(), cast and build a new unique_ptr if you are really sure what you are doing.
In general, if deleter<X> is convertible to deleter<Y> but does not handle Y properly, it might cause problems. So I'm not sure that this case is extraordinary in this sense.
I agree. If you declare deleter<X> is convertible to deleter<Y>, you declare it can delete objects of type Y after conversion.
It's true that these cases are a bit convoluted because, esp. in the presence of a user-defined pointer type, we need to decide how much to trust the deleter and how much to override whatever it says WRT conversions. For instance, let's say unique_ptr<X1,D1> wants to convert to unique_ptr<X2,D2> and D1::pointer is convertible to D2::pointer and D1 is convertible to D2 _but_ X2 does not have a virtual destructor. Do we disallow the conversion as we do in the ordinary X1* -> X2* case when using the default deleter, or do we go ahead? The array case is similar, unique_ptr<X[],D1> -> unique_ptr<Y[],D2>, X(*)[] is not convertible to Y(*)[] but both D1::pointer and D1 are convertible to D2::pointer and D2. Legal or not?
Semantics might be a bit more clear for default_delete, as it calls delete. std::is_polymorphic might not be very accurate, maybe std::has_virtual_destructor. A user-defined deleter could be a no-op or an operation that can properly recycle (link it in a intrusive list, etc.) the object even if it has no virtual destructor, just because it doesn't call the destructor. I think the implementation is nearly finished (has_virtual_destructor check is missing, as the standard does not require it, but it could be added). I've split tests and put all the meta utilities used by unique_ptr in a different header. Let me know if you feel some additional work must be done to push it into the "::boost" namespace and/or move it into smart_ptr. One missing decision is whether Boost.Move should be used in tests and/or use std::move/forward in C++11 compilers. And in case both approaches are testes in C++11 compilers, how we avoid duplicating test code. Ion
Ion Gaztañaga wrote:
Semantics might be a bit more clear for default_delete, as it calls delete. std::is_polymorphic might not be very accurate, maybe std::has_virtual_destructor.
has_virtual_destructor is the correct one. It's a later addition and the implementation on which I prototyped probably didn't have it.
A user-defined deleter could be a no-op or an operation that can properly recycle (link it in a intrusive list, etc.) the object even if it has no virtual destructor, just because it doesn't call the destructor.
In principle, we should leave everything to the deleter. The problem is that in practice things like the following often occur: struct X { }; struct Y: X { std::string s_; }; struct D { template<class T> void operator()( T* p ) const { delete p; } }; int main() { unique_ptr<Y, D> p1( new Y ); unique_ptr<X, D> p2( p1 ); // legal? } The array case is similar: struct E { template<class T> void operator()( T* p ) const { delete[] p; } }; int main() { unique_ptr<Y[], E> p1( new Y[3] ); unique_ptr<X[], E> p2( p1 ); // legal? } although here we do have grounds on which to reject - we know that operator[] will not work. I agree that the deleter may in principle not delete, and therefore the non-array conversion case may be safe. On the other hand, the obvious shared_ptr example, the one with the null deleter, doesn't translate, and I'm not sure I can come up with another (for which conversions are desirable).
El 03/09/2014 11:08, Peter Dimov escribió:
Ion Gaztañaga wrote:
Semantics might be a bit more clear for default_delete, as it calls delete. std::is_polymorphic might not be very accurate, maybe std::has_virtual_destructor.
has_virtual_destructor is the correct one. It's a later addition and the implementation on which I prototyped probably didn't have it.
Ok, this requires a compiler intrinsic, but I can add it to my unique_ptr implementation. Best, Ion
El 03/09/2014 13:16, Ion Gaztañaga escribió:
has_virtual_destructor is the correct one. It's a later addition and the implementation on which I prototyped probably didn't have it.
Ok, this requires a compiler intrinsic, but I can add it to my unique_ptr implementation.
One problem I've found for constructors taking the pointer is that the pointer can be an incomplete type whereas has_trivial_destructor requires a complete type. We could apply it to reset() and the assignment, but without constructors IMHO we miss the most used functions. Ion
El 03/09/2014 22:33, Peter Dimov escribió:
Ion Gaztañaga wrote:
One problem I've found for constructors taking the pointer is that the pointer can be an incomplete type whereas has_trivial_destructor requires a complete type.
Are we in the default_delete case here, or in the arbitrary deleter case?
I was trying in the default_delete case, sfinaeing out this constructor: template<class U, E> unique_ptr(unique_ptr<U, E> &&u, enable_if<... E ....>... ) As E must be convertible to D, and to check this, it was checking if T was a base class of U and had no virtual destructor, it seems that the intrinsic complained about non-complete types. Maybe I was not properly filtering it first with is_convertible. I've changed that approach to static_assert inside the function instead of sfinaeing the constructor. This seems to work. In any case, is expected is_convertible<T*, U*>::value to return false with pointers to incomplete types? Best, Ion
Ion Gaztañaga wrote:
In any case, is is_convertible<T*, U*>::value expected to return false with pointers to incomplete types?
No. is_convertible also wants complete types. It can't know whether U* is convertible to T* if it doesn't know what T is. It's not possible to use the converting constructor when T is incomplete - it would be an error either way. You can make it work for T* -> T const*, but not for U* -> T*.
Ion Gaztañaga wrote:
Would you agree making boost::movelib::unique_ptr boost::unique_ptr?
My dilemma is that, on one hand, I'm not very comfortable with your proposed implementation, while on the other, I suspect I won't be able to find the time to either produce a suitable alternative, or rework your submission appropriately. Which is a bit of a bind. Eventually, if I can't think of anything better to do, I'll have to accept your implementation so as to Not Block Progress. Still, here's my mini-review of it, to add some substance. * Tests - the include of <boost/move/detail/config_begin.hpp> should not be a part of the tests; tests should mirror ordinary use, and users aren't supposed to include detail headers. But more on config_begin/_end later. - the tests are too coarse-grained to my liking (the libc++ tests, in contrast, were too fine-grained :-). There are some obvious lines by which the tests can be split, one is by component, say: - default_delete - unique_ptr<T> - unique_ptr<T,D> - unique_ptr<T,D&> and the other is by whether the tests need to include a Boost.Move header. Some unique_ptr uses do spell out "boost::move" in places, others do not; some need to make a class movable, others do not. In addition, a test that needs to move can be written to assume C++11 and test using that (in addition, of course, to having the Boost.Move equivalent.) - incidentally, unique_ptr_functions.cpp doesn't seem to call unique_compare::test. * Implementation (I include everything #included in this review, and proceed roughly top to bottom.) - <boost/move/detail/config_begin.hpp>, _end.hpp: _CRT_SECURE_NO_DEPRECATE, _SCL_SECURE_NO_WARNINGS are project-level macros and I don't believe that a library should define them. In addition, these headers are nested, and I don't think that the mechanism that defines the macros handles this properly. Furthermore, I'm not sure I see anything in the guarded headers that actually needs these macros to be set. - <boost/move/detail/workaround.hpp>: This includes <boost/intrusive/detail/config_begin.hpp> and in general doesn't do much. - <boost/move/utility_core.hpp>: boost::move_detail::swap probably needs to not be in a detail namespace, as it's useful for writing swap functions (such as unique_ptr::swap.) - <boost/move/detail/meta_utils.hpp>: This header is a collection of useful type traits, but it contains both the type traits needed by Boost.Move proper, and those only needed by the unique_ptr implementation. It should be split so that the part that is only required by the unique_ptr implementation can go into smart_ptr if/when we move unique_ptr there. is_class_or_union is correctly named to reflect the implementation, but is actually used as if it were is_class, to control whether a type is derived from; so it should probably be called is_class. - Doxygen I understand that Doxygen makes the job of documenting much easier, but, in my opinion, it does make the code uglier with the #ifdef DOXYGEN_INVOKED, so I'd rather not use it. (We have enough ifdefs already for other reasons, and they add up.) - unique_ptr_data I don't understand how the deleter can be a pointer to a binary function. It's always called with one argument, the pointer. The rest are quibbles over the implementation that are of much less significance, so I'll stop here.
El 26/08/2014 18:40, Peter Dimov escribió:
Ion Gaztañaga wrote:
Would you agree making boost::movelib::unique_ptr boost::unique_ptr?
My dilemma is that, on one hand, I'm not very comfortable with your proposed implementation, while on the other, I suspect I won't be able to find the time to either produce a suitable alternative, or rework your submission appropriately. Which is a bit of a bind. Eventually, if I can't think of anything better to do, I'll have to accept your implementation so as to Not Block Progress.
Thanks Peter for your time. I can change the implementation, at least the most uncomfortable parts, to make No Block Progress nicer. I implemented boost::movelib::unique_ptr without the T[] specialization. I also cleaned up many enable_if ugly code, I was about to push it, but GCC 4.6 in C++11 mode ICEs without much explanation about what the front-end does not like. It compiles fine in several MSVC, GCC and Clang, though.
Still, here's my mini-review of it, to add some substance.
Great. I really appreciate it.
- the include of <boost/move/detail/config_begin.hpp> should not be a part of the tests; tests should mirror ordinary use, and users aren't supposed to include detail headers. But more on config_begin/_end later.
Ok.
- the tests are too coarse-grained to my liking (the libc++ tests, in contrast, were too fine-grained :-). There are some obvious lines by which the tests can be split, one is by component, say:
- default_delete - unique_ptr<T> - unique_ptr<T,D> - unique_ptr<T,D&>
Ok. If you feel that graining is correct, I will divide those tests. I ported tests quite quickly, avoid test class duplications (class A, class B, deleters, etc.) and implement the first working version as fast as possible. I didn't port the "fail" tests from Howard's test suite (e.g. test an invalid conversion is not accepted), and I still find these tests a bit unfocused (you don't know if the code failed to compile because the invalid conversion you wanted to test or due to any other reason).
and the other is by whether the tests need to include a Boost.Move header. Some unique_ptr uses do spell out "boost::move" in places, others do not; some need to make a class movable, others do not. In addition, a test that needs to move can be written to assume C++11 and test using that (in addition, of course, to having the Boost.Move equivalent.)
The nice thing about the tests is that you don't need different tests for C++03 or C++11 compilers. The test itself is portable as boost/move/unique_ptr.hpp is supposed to work in codebases to be compiled both in C++03 and C++11 environments. That at least helps maintaining the C++03 version in sync with the C++11 version and detect differences. Another option is to use Boost.Move or standard utilities depending on BOOST_NO_CXX11_RVALUE_REFERENCES. That would require duplicating some code in tests protected by #ifdefs, at least for class definitions. I don't think it's worth the effort, taking in care that Boost.Move machinery is really lightweight in C++11 compilers. In any case, if that's required to put it into Boost.SmartPtr, then we'll found a why to satisfy all these needs. Evidently, if tests are finer grained separated then we can push Boost.Move out of many tests. In any case, it will be included by boost/move/unique_ptr.hpp ;-)
- incidentally, unique_ptr_functions.cpp doesn't seem to call unique_compare::test.
Ouch. And there were errors due to copy pastes.
* Implementation
(I include everything #included in this review, and proceed roughly top to bottom.)
- <boost/move/detail/config_begin.hpp>, _end.hpp:
_CRT_SECURE_NO_DEPRECATE, _SCL_SECURE_NO_WARNINGS are project-level macros and I don't believe that a library should define them. In addition, these headers are nested, and I don't think that the mechanism that defines the macros handles this properly. Furthermore, I'm not sure I see anything in the guarded headers that actually needs these macros to be set.
Furthermore, #pragma warning (disable : 4996) should handle them, so I still don't remember why they ended there.
- <boost/move/utility_core.hpp>:
boost::move_detail::swap probably needs to not be in a detail namespace, as it's useful for writing swap functions (such as unique_ptr::swap.)
True. I'll put it into boost::movelib.
- <boost/move/detail/meta_utils.hpp>:
This header is a collection of useful type traits, but it contains both the type traits needed by Boost.Move proper, and those only needed by the unique_ptr implementation. It should be split so that the part that is only required by the unique_ptr implementation can go into smart_ptr if/when we move unique_ptr there.
This seems reasonable.
is_class_or_union is correctly named to reflect the implementation, but is actually used as if it were is_class, to control whether a type is derived from; so it should probably be called is_class.
Ok. This is not used by unique_ptr.
- Doxygen
I understand that Doxygen makes the job of documenting much easier, but, in my opinion, it does make the code uglier with the #ifdef DOXYGEN_INVOKED, so I'd rather not use it. (We have enough ifdefs already for other reasons, and they add up.)
Doxygen is OK for maintenance, and it's adequate while unique_ptr lives in Boost.Move, but obviously, it should be adapted to Boost.SmartPtr documentation. That could require removing Doxygen support. unique_ptr will be quite stable so code/documentation synchronization shouldn't be a problem.
- unique_ptr_data
I don't understand how the deleter can be a pointer to a binary function. It's always called with one argument, the pointer.
The trait was imported from Intrusive, where binary and unary functions are used in containers. I was just too lazy to rename/rework it.
The rest are quibbles over the implementation that are of much less significance, so I'll stop here.
Many thanks Peter. I will continue improving code and tests with your suggestions and ping the list again. Best, Ion
Ion Gaztañaga wrote:
Evidently, if tests are finer grained separated then we can push Boost.Move out of many tests. In any case, it will be included by boost/move/unique_ptr.hpp ;-)
This is an implementation detail. We may decide (either now or at some point in the future) to not include Boost.Move in C++11 mode at all in unique_ptr.hpp. And if we do that, it will help if we test whether both p1 = std::move( p2 ); and p1 = boost::move( p2 ); work (the former in C++11 mode, the latter in both 03 and 11 with appropriate #include <move/core_utility.hpp>.) Stated differently, I want boost::unique_ptr, on a C++11 compiler, to work without any inclusion or mention of Boost.Move in user code, if that's what the user wants (even though it wouldn't make much sense to not use std::unique_ptr there except perhaps for the <T[N]> extension, but still.)
I didn't port the "fail" tests from Howard's test suite...
We'll have to add fail tests, but I didn't mention that because it can be a later, incremental addition. shared_ptr has a number of those that check, f.ex. whether undesired conversions can occur.
I don't understand how the deleter can be a pointer to a binary function. It's always called with one argument, the pointer.
The trait was imported from Intrusive, where binary and unary functions are used in containers. I was just too lazy to rename/rework it.
It's also possible to use is_class instead. Technically, we also have to check is_empty, but in this case that's not strictly necessary. Something I missed in my post is that I think that default_delete should be in its own header (and its test should be named default_delete_test.) In principle, it might be good to also have tests for the type traits, even though they are implementation details. But that can wait until we stabilize the contents of the type traits header. Incidentally, addressof is in core. Is Boost.Move so completely free of dependencies now so that even core is considered too much?
El 26/08/2014 23:36, Peter Dimov escribió:
Ion Gaztañaga wrote:
Evidently, if tests are finer grained separated then we can push Boost.Move out of many tests. In any case, it will be included by boost/move/unique_ptr.hpp ;-)
This is an implementation detail. We may decide (either now or at some point in the future) to not include Boost.Move in C++11 mode at all in unique_ptr.hpp. And if we do that, it will help if we test whether both
p1 = std::move( p2 );
and
p1 = boost::move( p2 );
work (the former in C++11 mode, the latter in both 03 and 11 with appropriate #include <move/core_utility.hpp>.)
We can execute the test with some macro specifying (in C++11) if we want to test it using boost move or not. That would cover both C++11 and Boost.Move users writing portable C+++03/C++11 code.
Stated differently, I want boost::unique_ptr, on a C++11 compiler, to work without any inclusion or mention of Boost.Move in user code, if that's what the user wants (even though it wouldn't make much sense to not use std::unique_ptr there except perhaps for the <T[N]> extension, but still.)
That's possible at this moment, although it is not being tested.
Something I missed in my post is that I think that default_delete should be in its own header (and its test should be named default_delete_test.)
While I haven't split this into another header, the latest commit, removes the array specialization, tidies doxygen macros with other macro utilities and the readability of the implementation has been improved as the header has much less code: https://github.com/boostorg/move/blob/develop/include/boost/move/unique_ptr.... unique_ptr is also now constructible and assignable from zero in c++03 code. Tested in several GCC, MSVC and Clang versions, both in C++03 and C++11 modes. I won't have time in the following days to continue working in your proposed changes, but at least I wanted to to show that we've progressed a bit. Best, Ion
Ion Gaztañaga wrote:
Tested in several GCC, MSVC and Clang versions, both in C++03 and C++11 modes. I won't have time in the following days to continue working in your proposed changes, but at least I wanted to to show that we've progressed a bit.
Thanks! There's one other thing that I omitted in my last post: 85 template <class U> 86 unique_ptr_data(P p, BOOST_FWD_REF(U) d) BOOST_NOEXCEPT 87 : m_p(::boost::forward<U>(p)), d(::boost::forward<U>(d)) 88 {} It's not clear to me why you need this constructor (in addition to the two others), and it's wrong - forward<U>(p) isn't likely to work. In my implementation, I've went with perfect forwarding instead of the two constructor signatures described in the standard. Combined with the static_assert inside, It works for all cases. Not sure how this would translate to C++03 though; what does BOOST_FWD_REF(U) expand into? C++03 can't really do perfect forwarding.
In my implementation, I've went with perfect forwarding instead of the two constructor signatures described in the standard.
Actually, I'm not sure how you implement the standard requirement in C++03. If you do that: The signature of these constructors depends upon whether D is a reference type. If D is non-reference type A, then the signatures are: unique_ptr(pointer p, const A& d); unique_ptr(pointer p, A&& d); (and it seems that you do) deleter rvalues will always bind to A const& in C++03, never to rv<A>, as we saw in the recent thread. So it seems that unique_ptr<T>( p, D() ) will fail for a movable, noncopyable, D. unique_ptr<T>( p, move(d) ) will work though.
El 28/08/2014 2:02, Peter Dimov escribió:
unique_ptr(pointer p, const A& d); unique_ptr(pointer p, A&& d);
(and it seems that you do) deleter rvalues will always bind to A const& in C++03, never to rv<A>, as we saw in the recent thread. So it seems that
unique_ptr<T>( p, D() )
will fail for a movable, noncopyable, D.
unique_ptr<T>( p, move(d) )
will work though.
Yes, thanks for pointing out that. For movable only types in C++03 using Boost.Move, we can detect that using an internal tag and change the signature to something that accepts rvalues, maybe (not tested). I'll add this to the to-do list. Best, Ion
El 28/08/2014 1:13, Peter Dimov escribió:
Ion Gaztañaga wrote:
Tested in several GCC, MSVC and Clang versions, both in C++03 and C++11 modes. I won't have time in the following days to continue working in your proposed changes, but at least I wanted to to show that we've progressed a bit.
Thanks!
There's one other thing that I omitted in my last post:
85 template <class U> 86 unique_ptr_data(P p, BOOST_FWD_REF(U) d) BOOST_NOEXCEPT 87 : m_p(::boost::forward<U>(p)), d(::boost::forward<U>(d)) 88 {}
It's not clear to me why you need this constructor (in addition to the two others), and it's wrong - forward<U>(p) isn't likely to work.
Apart from the useless "forward<>()" (which I think will work as pointers are not move-enabled), if I disable this overload some compilers complain about move-only deleters not being copyable in C++03. I haven't investigated the issue, it might be provoked from some SFINAE provoked instantiations or compiler bugs. I'll try to simplify this if I find a way.
In my implementation, I've went with perfect forwarding instead of the two constructor signatures described in the standard. Combined with the static_assert inside, It works for all cases. Not sure how this would translate to C++03 though; what does BOOST_FWD_REF(U) expand into? C++03 can't really do perfect forwarding.
BOOST_FWD_REF(U) expands to const U &, which means that non-const references can't be forwarded to constructors. That could be somewhat workarounded using an utility like boost::ref internally, but I think deleter_arg_type1/2 allow a more portable C++03/C++11 implementation. Best, Ionest, Ion
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Ion Gaztañaga wrote:
85 template <class U> 86 unique_ptr_data(P p, BOOST_FWD_REF(U) d) BOOST_NOEXCEPT 87 : m_p(::boost::forward<U>(p)), d(::boost::forward<U>(d)) 88 {}
It's not clear to me why you need this constructor (in addition to the two others), and it's wrong - forward<U>(p) isn't likely to work.
Apart from the useless "forward<>()" (which I think will work as pointers are not move-enabled),
Unless forward<U> ignores U, I don't see how it would work. The type of the pointer is P, not U, which is not likely to be the same.
participants (6)
-
Adam Wulkiewicz -
Agustín K-ballo Bergé -
Glen Fernandes -
Ion Gaztañaga -
Peter Dimov -
Vicente J. Botet Escriba