Issue123movesupport

[ecorm]

Implemented C++11 move semantics for GenericDocument, as well as related unit tests. This is related to issue #123.

[pah]

In general, it looks good to me (not tested yet).

But I would prefer to merge this only after the default MemoryPoolAllocator is Movable as well.

[ecorm]

This static_cast in GenericDocument's move assignment operator...

ValueType::operator=(std::move(static_cast<ValueType&&>(rhs)));

...was necessary, because otherwise it would attempt to call GenericValue's templated assignment operator.

GeneralDocument contains the Allocator by pointer, not by value, so there's no need to make the Allocator movable. When a move occurs, it's simply moves the pointers to the Allocator. Therefore, a GenericDocument using a MemoryPoolAllocator is also movable. I have made the GenericDocument move unit tests use both CrtAllocator and MemoryPoolAllocator just to be sure.

All of the other classes contain the Allocator by pointer, so there is currently no need to make MemoryPoolAllocator movable. But if you wish, I can also make the MemoryPoolAllocator movable in case someone later decides to contain it by value instead of by pointer.

[pah]

This static_cast in GenericDocument's move assignment operator...

ValueType::operator=(std::move(static_cast<ValueType&&>(rhs)));

...was necessary, because otherwise it would attempt to call GenericValue's templated assignment operator.

Good catch. move+static_cast<...&&> is still a redundant. We could use std::forward instead, which does an explicit rvalue-ref cast (comment should still stay, of course):

ValueType::operator=(std::forward<ValueType>(rhs));

Ok for skipping the allocator move (my misconception). Thanks for testing MemoryPoolAllocator to be sure.

Thanks for your effort! This should fix #123. 👍

[ecorm]

I've made the std::forward change that you suggested.

[pah]

Thanks! From my side, it's good for merging now. 👍

[ecorm]

Unless someone else has any suggestions to make, I don't plan on making any further changes for this pull request.

[ecorm]

I have done some research into the proper, idiomatic way of using std::move versus using std::forward.

std::move is intended be used when the intent is to unconditionally convert a parameter/variable to an rvalue reference of the same type (in order to propagate the rvalueness when implementing move semantics).

std::forward is intended to be used for perfect forwarding within templated functions, where the type-deduced parameter is what Scott Meyers calls a "universal reference".

See:

• http://stackoverflow.com/questions/13219484
• http://channel9.msdn.com/Shows/Going+Deep/Cpp-and-Beyond-2012-Scott-Meyers-Universal-References-in-Cpp11 (at 0:30:38)
• http://www.codingstandard.com/rule/17-3-2-use-stdforward-to-forward-universal-references/

So while:

ValueType::operator=(std::forward<ValueType>(rhs));

happens to work correctly within the context of GenericDocument::operator=(GenericDocument&&), it does not make idiomatic sense, because the parameter is not a "universal reference".

That line should really be:

ValueType::operator=(static_cast<ValueType&&>(rhs));

because we want to unconditionally cast rhs to an rvalue reference of the base type.

Unless there are compelling reasons why I shouldn't, I'd like to change the GenericDocument move assignment so that it uses:

ValueType::operator=(static_cast<ValueType&&>(rhs));

[pah]

I humbly disagree that std::forward doesn't make sense here. Yes, the main use-case for std::forward is perfect forwarding. OTOH, the main difference to std::move is the need for giving the explicit template parameter.

A direct cast to ValueType&& would work as well, of course (as this is the only thing, std::forward and std::move do internally). But personally, I find std::forward easier to read than static_cast<ValueType&&>.

But at the end of the day, I don't care much.

NB: Herb Sutter and others suggest the name "forwarding reference" instead of "universal reference" (see Herb's talk at CppCon'14).

[ecorm]

I don't care much either whether it's static_cast<ValueType&&> or std::forward<ValueType>; either one will work. I don't want my "nitpicking" to hold back merging of the pull request, so I'll leave things as they currently are.
P.S. Thanks for the vid link!

[miloyip]

I am not very familiar with C++11 actually.
But it seems the current situation is fine for both of you.
Thank you for your contribution.