Use move instead of copy in vector::insert()/set_capacity()

[Wawha]

In vector::insert() and vector::set_capacity(), a copy is made on existing elements. Use move semantic in order to avoid copy when elements are moved or vector resize.

Here an example of code which show the extra copy done:

struct CheckCopy
{
	CheckCopy() : m_Value(0) {}
	CheckCopy(int value) : m_Value(value) {}
	CheckCopy(const CheckCopy& iOther) : m_Value(iOther.m_Value) { std::cout << "Copy" << std::endl; }
	CheckCopy(CheckCopy&& iOther) : m_Value(iOther.m_Value) {}
	void operator =(const CheckCopy& iOther) { m_Value = iOther.m_Value; std::cout << "Copy" << std::endl; }
	void operator =(CheckCopy&& iOther) { m_Value= iOther.m_Value; }

	int m_Value;
};

void TestInsert()
{
	eastl::vector<CheckCopy> myVector1;
	myVector1.reserve(20);
	for (int idx = 0; idx < 2; ++idx)
		myVector1.push_back().m_Value = idx;

	eastl::vector<CheckCopy> myVector2;
	for (int idx = 0; idx < 3; ++idx)
		myVector2.push_back().m_Value = 10 + idx;

	// Here we have 5 copy instead of 3
	std::cout << "Insert 3 elts with iterator" << std::endl;
	myVector1.insert(myVector1.begin(), myVector2.begin(), myVector2.end());

	eastl::vector<CheckCopy> myVector3;
	myVector3.push_back().m_Value = 20;
	// Here we have 6 copy instead of one
	std::cout << "Insert 1 elt with iterator" << std::endl;
	myVector1.insert(myVector1.begin(), myVector3.begin(), myVector3.end());

	// Here we have 8 copy instead of 2
	std::cout << "Insert 1 elt" << std::endl;
	myVector1.insert(myVector1.begin(), 1, CheckCopy(17));

	// Here we have 18 copy instead of 11
	std::cout << "Insert 10 elts" << std::endl;
	myVector1.insert(myVector1.begin(), 10, CheckCopy(17));

	// No copy should be done here.
	std::cout << "set_capacity(2)" << std::endl;
	myVector1.set_capacity(2);
}

Here the associated issue: #67

[rparolin]

Thanks for submitting this PR.

Please include a test that highlights the issue and demonstrates that it is resolved by your change. The code you provided as an example is a good start. I'd recommend reading the existing test code in EASTL that uses 'TestObject' as an example of what I'm looking for. 'TestObject' is a test utility that counts the calls to the various constructors and operators and it is used to validate the behaviour of EASTL containers.

Example:
https://github.com/electronicarts/EASTL/blob/master/test/source/TestVector.cpp#L538

[Wawha]

Hi,
Thank you for the advices. I resubmit my commit with some tests to demonstrate & validate the changes I made.
It's help me to fix a problem in the first implementation of set_capacity() I made!

Note that existing tests was failing with my first implementation of set_capacity(), but the continuous integration, did not failed (see: https://ci.appveyor.com/project/rparolin/eastl/build/1.0.128/job/jsfvpy3au5r1p095#L758). The executable test return code 1 (https://ci.appveyor.com/project/rparolin/eastl/build/1.0.128/job/jsfvpy3au5r1p095#L863), but after that EASTLBenchmarks.exe was launched and return 0. And it seems that it's only this last error code which is used.

About tests, I also add few #if EASTL_MOVE_SEMANTICS_ENABLED/#endif in TestVector.cpp in order to be able to build without this define.

Tell me if you see some problem or have remark about the code or test.

[rparolin]

I JUST noticed that myself. I guess I have to add 'bundle' commands to my .travis.yml file. Thanks for bringing that up.

https://docs.travis-ci.com/user/customizing-the-build/#Customizing-the-Build-Step

[rparolin]

Please align pre-processor conditionals with the braces.

[Wawha]

Ok, I have recommit this part, moving the pre-processor on the previous line in order to have the same style has the other cases.

[rparolin]

I believe previously you simply utilized vector's move constructor. What was the problem with that implementation? I don't believe this above is required to be written explicitly.

[Wawha]

Yes, in my first implementation, I just used the move constructor with eastl::move(). With that, I have few UnitTest failed, because the move was done on the buffer, not one each element, so the the allocation at the end stays with the exact same size!
While the set_capicity() ask to reduce the size of the buffer/container to fit the size, it was not correct.

I'm agree that this implementation is quite long, and perhaps there is other internal vector I could call to reduce the code size. If you have idea for that, tell me.

[rparolin]

Take a look at the 'shrink_to_fit' implementation. It utilizes the vector constructor which takes a pair of iterators (begin, end) and passes move iterators to that constructor.

    #if EASTL_MOVE_SEMANTICS_ENABLED
        this_type temp = this_type(move_iterator<iterator>(begin()), move_iterator<iterator>(end()), mAllocator);

[Wawha]

Indeed shrink_to_fit() function seems to do the job. And the exact same job as it's needed here. So I remove the part of code which create a temp vector and swap it and just call shrink_to_fit().
Tell me if it's all ok for you.

[rparolin]

Thanks for the PR. :)