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

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

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

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

- :

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