Adding a full C++11 feature : move constructor and move assignment operator for vector

[gnthibault]

I did a very simple, yet working implementation of move semantic, that is not the default c++11 implementation.

Here are some question you may ask:

Why C++11 default implementation is not good ?
-There is not implicit default move semantic for all thrust vector class because copy constructor, copy assignment operator and destructor are already user-declared, see (http://stackoverflow.com/questions/4819936/why-no-default-move-assignment-move-constructor) for references

Why defaulted C++11 implementation is not good ?
-because it does not necessarily initialize the "void" new object with its default constructor, depending on the type of the pointer/size member it can even just make a copy.
If a copy is performed, when deleting the temporary object, weird behaviour can occur, ie, pointer or size of the object moved from can keep their old value or take random values, and we may delete either a random pointer or more probably the valid pointer that as been copied in the move operation /!.

Why so few modifications ?
-because the swap operator was already defined in vector_base
-because device_vector and host_vector does not define their own members attributes.

Why did you defined assignment operator in device_vector ?
-because it wasn't defined before, and because, defining the move assignment operator automatically makes it deleted.

Why do the unit test does not execute when I build ?
-I did not found example of how to add pure c++11 test in the scons script, so I modified it by hand, by adding

my_env.Append(CPPFLAGS = '-std=c++11')
in testing/SConscript line 23

and I get the following results under linux:

[PASS]          0 ms TestVectorMoveSemanticDevice
[PASS]          0 ms TestVectorMoveSemanticHost

[jaredhoberock]

Thanks Thibault!

After taking a quick look at the diff, it looks like there are some spurious whitespace changes included with this pull request. Would you mind reverting the whitespace changes? It makes it easier to review the actual content of the pull request without the noisy whitespace changes showing up in the diff.

[3gx]

To avoid undefined behaviour at runtime, would it be rather better to assert(this != &v).

[gnthibault]

Ok, thank you for your feedback, and sorry for the messy commit, it appeared that my editor (atom) corrected many end of line space, and end of file newlines...

egaburov, I took into account you remark and added the self move assignment runtime assert check.

Anyway, regarding this particular aspect my comment in the previous pull request was misleading, there was no undefined behaviour per se, but a behaviour that may seems weird although compliant with move semantic:
A statement like this one:
obj = std::move(obj)
would always result in obj containing a default constructed version, which is a valid state.

It seems that performing the self assignement check in the move assignment operator is a design choice, at least I was not able to find any specific information about the expected behaviour of self move assignement in the standard:
-free draft from Nov 2014 : see page 305-310 of the pdf
-See also this SO post

Tests are passing using my modified version of the sconscript (not commited), but I still don't know how to enable the -std=c++11 for c++11 test build properly.

[3gx]

I still don't know how to enable the -std=c++11 for c++11 test build properly

Add std=c++11 to scons line, e.g. scons std=c++11 arch=sm_50 run_unit_tests -j8

[jaredhoberock]

Here is a comment from Howard Hinnant on self-moves to standard library types: http://stackoverflow.com/questions/13127455/what-does-the-standard-library-guarantee-about-self-move-assignment

[jaredhoberock]

We may as well unconditionally #include <utility>, no matter the C++ version, since the header will always exist.

Eliminating the #if guard here (and in the other files it is used) should help reduce confusion.

[jaredhoberock]

Thanks for cleaning up the diff Thibault, it's much easier to review now!

[gnthibault]

Ok I modified the comments that suffered from savage copy/pasting.
Concerning the self move assignment related post on SO, by Howard Hinnant seems to suggest that no check should be performed for self move assignment.

So I wonder if the assert check should still be part of the vector_base move assignment, it is important to notice that it would probably differs from most of the the std::vector implementation behaviour and break some valid c++ code at runtime like this one :

#include <vector>
#include <utility>
#include <algorithm>
#include <iostream>

//Compile using g++ ./main.cpp -std=c++11 -o ./test
int main()
{
  std::vector<int> v(10,5555);
  std::cout << "Size of vector before move is "<<v.size()<<std::endl;
  v = std::move(v);  
  std::cout << "Size of vector is now "<<v.size()<<std::endl;
  std::for_each(v.cbegin(),v.cend(),[](int in){ std::cout<<in<<std::endl;});
  // vector is now void
  return EXIT_SUCCESS;
}

My opinion is that it should be removed, but I am waiting for your decisions.

[jaredhoberock]

My interpretation of Howard Hinnant's post is that the standard says that self-moves to standard library types have undefined behavior. I think that means we can use the assert to check for self-moves, or not; it's up to us. I don't have a strong opinion either way; if @egaburov feels strongly that the assert will catch programmer errors, then we should leave it in.

As a bookkeeping item, we need to note this addition in the CHANGELOG and give Thibault credit. @gnthibault, could you add an appropriate note about the move constructors and move assignment operators introduced into host_vector and device_vector, and also credit yourself for the work?

[jaredhoberock]

Also: there are thrust::system::cpp::vector, thrust::system::omp::vector, thrust::system::tbb::vector, and thrust::system::cuda::vector.

Do we think that these vector types also require the additions?

[3gx]

I don't have any strong feelings about adding assert there, it was a suggestion triggered by seeing undefined behaviour comment. I have no objections if you think it should be removed.

[gnthibault]

I decided to remove the assert for self move assignment, in order to mimic the behaviour of std::vector implementation on linux distributions with gcc.
I added a few lines on the Changelog, and credited myself.

I added the support for move semantic in thrust::system::cpp::vector, thrust::system::omp::vector, thrust::system::tbb::vector, and thrust::system::cuda::vector.

But I did not figured out if there was specific unit test that should be written for these specific implementations to be validated ?

[jaredhoberock]

Thanks Thibault!

I added the support for move semantic in thrust::system::cpp::vector, thrust::system::omp::vector, thrust::system::tbb::vector, and thrust::system::cuda::vector.

But I did not figured out if there was specific unit test that should be written for these specific implementations to be validated ?

I don't think we have unit tests for the backend-specific vectors since they are so similar to host_vector & device_vector. We may decide to add them someday, and we can test the move functionality if/when we do so.

I think this pull request is ready to merge. @egaburov, what do you think?

[3gx]

There is a compilation failure with one of the examples on Linux:

$ nvcc examples/cuda/custom_temporary_allocation.cu -I. -arch=sm_50
$

$ nvcc examples/cuda/custom_temporary_allocation.cu -I. -arch=sm_50 -std=c++11
examples/cuda/custom_temporary_allocation.cu(149): error: function "thrust::system::cuda::vector<T, Allocator>::operator=(const thrust::system::cuda::vector<int, thrust::system::cuda::allocator<int>> &) [with T=int, Allocator=thrust::system::cuda::allocator<int>]" (declared implicitly) cannot be referenced -- it is a deleted function

1 error detected in the compilation of "/tmp/tmpxft_00002d3e_00000000-9_custom_temporary_allocation.cpp1.ii".

[jaredhoberock]

It sounds like the backend-specific vector types will need a user-provided copy assignment operator (just like host_vector and device_vector). Otherwise, the compiler will delete the copy assignment operator because it sees the user-provided move assignment operator.

Thibault, can you introduce a copy assignment operator for each of these vectors and add it to your pull request?

[gnthibault]

Sorry for the multiple modifications ...
A short description of my last commit:

To avoid an important code duplication, and to keep the current behaviour as much as possible in the regular case (no move semantic applicable), I defaulted all the copy assignment operator in the backend classes, so they are no more deleted.

Now all examples and unit tests are passing, I also wrote an ultra simple test script (linux) that tests for move semantic on all backends.
(rename main.txt as main.cu)

main.txt

As suggested earlier by Jared, I also moved the regular copy assignment operator of device_vector from C++11 guard to generic case.
My first intent was to keep this definition only for the C++11 support in order not to modify non-C++11 behaviour but then I realized that redefining this behaviour was more consistent with the host_vector code.

Another option is to make both the host_vector and the device_vector homogeneous copy assignment operator=() defaulted, as they will call their parent's anyway.

[jaredhoberock]

@gnthibault, thanks for the additional work and due diligence. Let's go ahead and implement operator= consistently in all the vector classes. I'll leave it up to you to decide whether or not using default is the best way to do it. After this one last modification, I think we'll be ready to merge.

[gnthibault]

You are welcome! unless you would like more thorough unit testing, I am ok with the current version.

I think it would also be interesting to run example/tests under windows, but I haven't got any windows platform available for the moment.

[jaredhoberock]

Thanks Thibault. What I meant was, the last change that should be applied would be to make sure that operator= has a consistent implementation across all the different vector classes that this pull request touches. It looks like some of the implementations of operator= are user-defined, and some of them have a = default; implementation.

Can these operator= functions all be implemented consistently (either all with a user-defined implementation, or all with a default implementation)?

[gnthibault]

I re-arranged some method definitions order to be more consistent with all vectors definitions.

I made all definitions in the derived classes (move construction, copy construction, move assignment, move assignment) explicit, with explicit implementation.

This indeed make the code somehow more "consistent" so you don't have to ask why some methods are implemented, for instance the one that are templated, or take std::vector as input, and some other are defaulted.

But, for all derived classes, ie device_vector, host_vector, cpp,cuda,omp, and tbb backends, all of the implementation I wrote correspond to the default one, and therefore could have been defaulted.

This would have resulted in less code to maintain in derived class, but maybe less readable code.

I let you decide if the current state satisfies thrust philosophy.

Thank you for your comments

[jaredhoberock]

Thanks @gnthibault! The only eyebrow raiser I see in the current change is that the calls to std::forward used in the implementation of the move constructors should be calls to std::move. I think these end up equivalent, however, in the way that they are used in your current code.

I'm going to go ahead and merge this pull request and fix it up in a separate commit.

Thank you very much for your patience and hard work!