Overloading operator to merge vectors

Question

I want to overload the operator + in a class which has a vector as member variable, so as to carry out the merging of vectors of two different objects. In other words I would like a new object to be created which has as vector both the elements of the vectors of the two initial vectors in a row. I try the following code and I receive an error in operator+ regarding std::copy. What is the problem?

class TestVector
{
    std::vector<int> myVector;

public:
    TestVector(){};
    TestVector(std::vector<int>);
    std::vector<int> getVector();
    TestVector operator +(TestVector);

};

std::vector<int> TestVector::getVector()
{
   return myVector;
}

TestVector TestVector::operator+(TestVector param)
{

    std::vector<int> tempVector;
    std::vector<int> paramVector = param.getVector();

    std::copy(paramVector.begin(), paramVector.end(), tempVector);
    std::copy(myVector.begin(), myVector.end(), tempVector.end());

    TestVector TestVector1(tempVector);

    return TestVector1;

}

Moreover, is the second copy statement valid to merge two vectors in general?

Update: I got an error at execution time saying that iterators are inompatible at this statement. What is wrong?
tempVector.insert(tempVector.end(),param.getVector().begin(), param.getVector().end());

Answer 1

If you are going to implement operator+ for your type, I would recommend that you also implement operator+= as a member function and then operator+ in terms of the previous operator:

// As a public member function
TestVector& TestVector::operator+=( const TestVector& rhs ) {
   myVector.insert( myVector.end(), rhs.myVector.begin(), rhs.myVector.end();
   return *this;
}

TestVector operator+( TestVector lhs, const TestVector& rhs ) {
   lhs += rhs;
   return lhs;
}

The advantage is that you only need a couple more lines of code and you get a new operation for free, it does not require friendship or providing accessors to the internal details of the type and is efficient (does not require copying) for the case where the user code does not need to maintain the left hand side operator. At the same time, the implementation of operator+ can be done as a free function (symmetry with respect to types --i.e. conversions ¹) and efficient (in a C++11 compiler it will copy each element only once if TestVector has a move constructor which it should since the only member is a vector that is movable; in C++03 there is an extra copy in the return statement that can be removed by deafult constructing and swapping).

¹ Note: you might want to consider making the TestVector( std::vector<int> const & ) explicit to avoid implicit conversions to TestVector.

Answer 2

The third argument to std::copy must be an iterator, not a container. You can use std::back_inserter thus:

std::copy(paramVector.begin(), paramVector.end(), std::back_inserter(tempVector));

The second copy will compile, since std::end() returns an iterator, but it will break because the iterator simply points to the end of the vector. It won't grow the vector when std::copy tries to assign to it; it will simply invoke undefined behaviour. Again, std::back_inserter solves the problem.

In any event you don't need std::copy at all, since std::vector provides the necessary semantics directly:

TestVector TestVector::operator+(const TestVector& v)
{
    std::vector<int> t(myVector);
    t.insert(t.end(), v.myVector.begin(), v.myVector.end());
    return TestVector(t);
}

And remember to use const &… to avoid so much copying. For instance, the second constructor copies the input vector unnecessarily, and getVector() copies the internal vector unnecessarily.

Even the above solution copies the temporary vector at least once, which can be avoided with a purpose-built constructor:

class TestVector
{
    std::vector<int> myVector;

public:
    TestVector() { }
    TestVector(const std::vector<int>& v) : myVector(v) { }
    const std::vector<int>& getVector() const { return myVector; }
    TestVector operator+(const TestVector& v) const {
        return TestVector(op_add(), *this, v);
    }

private:
    struct op_add { };
    TestVector(op_add, const TestVector& a, const TestVector& b) : myVector(a) {
        myVector.insert(myVector.end(), b.myVector.begin(), b.myVector.end());
    }
};

Answer 3

The third paramter to std::copy should be some sort of output iterator. The safest thing to use would be an insert iterator. After a #include <iterator> you should be able to do something like:

std::copy(paramVector.begin(), paramVector.end(), std::back_inserter(tempVector));
std::copy(myVector.begin(), myVector.end(), std::back_inserter(tempVector));

std::back_inserter is a helper function template that creates the appropriate back_insert_iterator for the given container.

Also note that you are making quite a lot of copies. Personally, I would make operator+ a free function and take the parameters by const reference. At the moment your operator+ is non-const, so can only be called on non-const vectors and takes its second parameter by copy. If getVector were declared: const TestVector& getVector() const; then you could do something like this.

TestVector operator+(const TestVector& lhs, const TestVector& rhs)
{
    std::vector<int> ret( lhs.getVector() );
    ret.insert( ret.end(), rhs.getVector().begin(), rhs.getVector().end() );
    return TempVector(ret);
}