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.