Boost logo

Boost :

Subject: Re: [boost] [move][unique_ptr] c++14 unique_ptr comes to town
From: Peter Dimov (lists_at_[hidden])
Date: 2014-08-26 12:40:57

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

* Implementation

(I include everything #included in this review, and proceed roughly top to

- <boost/move/detail/config_begin.hpp>, _end.hpp:

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.

Boost list run by bdawes at, gregod at, cpdaniel at, john at