Subject: Re: [boost] [move][unique_ptr] c++14 unique_ptr comes to town
From: Ion Gaztañaga (igaztanaga_at_[hidden])
Date: 2014-08-26 17:11:13
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
> Still, here's my mini-review of it, to add some substance.
Great. I really appreciate it.
> - 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&>
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
> 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
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
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
> - incidentally, unique_ptr_functions.cpp doesn't seem to call
Ouch. And there were errors due to copy pastes.
> * 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.
Furthermore, #pragma warning (disable : 4996) should handle them, so I
still don't remember why they ended there.
> - <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.)
True. I'll put it into boost::movelib.
> - <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.
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
> - 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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk