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 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
- unique_ptr
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.)
- , _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.
- :
This includes and in general
doesn't do much.
- :
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.)
- :
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.
