Re: [boost] [move][unique_ptr] c++14 unique_ptr comes to town

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