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: 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 
Clang, though.

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

Ok.

> - 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 
other reason).

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

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

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 
boost/move/unique_ptr.hpp ;-)

> - incidentally, unique_ptr_functions.cpp doesn't seem to call
> unique_compare::test.

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

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

Best,

Ion