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

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 and unique_ptr 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

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

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

El 26/08/2014 18:54, Peter Dimov wrote:

Mine is "more complete" in that it supports unique_ptr 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, 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 -> unique_ptr 2) unique_ptr -> unique_ptr 3) unique_ptr -> unique_ptr if remove_cv_t == remove_cv_t, 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 is be convertible to deleter 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 -> unique_ptr 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

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

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 wants to convert to unique_ptr 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 -> unique_ptr, 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

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

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?

Ion Gaztañaga wrote:

In any case, is is_convertible::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*.

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

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

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

- :

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

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

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

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

El 28/08/2014 22:26, Peter Dimov escribió:

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.

Touché! Ion