C++, how to correctly copy std::vector in copy constructor?

Question

I'm using this two classes

// This is generic data structure containing some binary data
class A {
public:
    A();
    A(const A&);
    ~A();
}

// Main data container
class B {
public:
    B();
    B( const B&);
    ~B();
protected:
    std::vector<A *> data;
}

// Copy constructor for class b
B::B( const B& orig):data() {
    for( std::vector<A *>::const_iterator it = orig.data.begin();
        it < orig.data.end(); ++it){
        data.push_back( new A( *(*it)));
    }
}

I guess this class would do its job, but I'm looking for a way to reach total perfection.

First, :data() - is this initialization required to initialize empty vector correctly (is it clean code)?

How is vector::iterator used in copy constructor? The only way I found is the one I've written into code (const should be mandatory for copy constructor).

Does copying the vector copy pointer values and not whole objects?

And finally new data initialization... Is there any way to replace the whole loop with less code and/or is there any standard how to write copy constructor for std::containers which contain object pointers?

Sub question: I'm assuming using vector<A *> is much more suitable and effective for various reasons than just vector<A> (not copying every time, power to decide whether (not) to copy objects...) Is this assumption correct?

Did you mean "Sub question: I'm assuming using a vector of pointers..." — Vaughn Cato, Jan 14 '12 at 21:56
Preallocate `data` in initializer list. Using `push_back()` like that is very ineffective. — lapk, Jan 14 '12 at 21:56
Sub answer: I think, if you _can_ not to use pointers, you _shouldn't_ use them. — Lol4t0, Jan 14 '12 at 21:58
I never used boost, but I am sure that it has a solution for this problem (it always has solutions for problems that seem common). — Sergey Kalinichenko, Jan 14 '12 at 22:03
As the accepted answer shows, getting this right with all the exception handling is a pain. A vector of smart pointers or a `std::deque` of objects is usually better. (Also, for small objects, a vector of pointers is typically slower than a vector of objects; too many indirections and calls to new/delete) — Nemo, Jan 15 '12 at 00:21

score 11 · Accepted Answer · edited Jun 28 '21 at 08:22

11

The data() is not necessary because that will be done automatically to the vector before the constructor is entered. You only need to initialise members that are POD (plain old data) types or types which have no default constructor (or references, constants, etc).

You can initialise the vector with the number of elements that the other one has, so that the vector doesn't have to resize itself as it grows. If you don't, you're starting with a small vector and making it incrementally reach the destination size via allocations and reallocations. This will make the vector the correct size from the very beginning:

B::B(const B& orig) : data(orig.data.size()) {
    for (std::size_t i = 0; i < orig.data.size(); ++i)
        data[i] = new A(*orig.data[i]);
}

Notice that you are not using push_back any more because the vector is already full of orig.data.size() number of elements that are default constructed (which is NULL in the case of pointers).

This also trims down the code because you can use an integer to iterate it instead of an iterator.

If you really want to use iterators, you can do

B::B(const B& orig) : data(orig.data.size()) {
    // auto is preferable here but I don't know if your compiler supports it
    vector<A*>::iterator thisit = data.begin();
    vector<A*>::const_iterator thatit = orig.data.cbegin();

    for (; thatit != orig.data.cend(); ++thisit, ++thatit)
        *thisit = new A(**thatit);
}

The advantage of this is that it will work with other container types (like list) by just changing the types of the iterators (but of course that would go away if you have auto).

If you want to add exception-safety, you need a try/catch block:

B::B(const B& orig) : data(orig.data.size()) {
    try {
        // auto is preferable here but I don't know if your compiler supports it
        vector<A*>::iterator thisit = data.begin();
        vector<A*>::const_iterator thatit = orig.data.cbegin();

        for (; thatit != orig.data.cend(); ++thisit, ++thatit)
            *thisit = new A(**thatit);
    } catch (...) {
        for (vector<A*>::iterator i = data.begin(); i != data.end(); ++i)
            if (!*i)
                break;
            else
                delete *i;

        throw;
    }
}

This way you will not have a memory leak if one of the new calls throws an exception. Of course you can use the try/catch along with the way without iterators if you'd rather do it that way.

edited Jun 28 '21 at 08:22

Adam

77
2
12

answered Jan 14 '12 at 21:55

Seth Carnegie

73,875
22
181
249

1

You need to dereference `orig.data[i]`. – someguy Jan 14 '12 at 21:58
2

One more thing to take into consideration is error handling: what happens if one of the `new`s fail in the middle of the loop? Probably the program will just terminate, but if the out of memory error is handled further up in the call stack you'll have a memory leak. – Emil Styrke Jan 14 '12 at 22:01
@EmilStyrke please review my latest edit to see if that handles the situation you are talking about. – Seth Carnegie Jan 14 '12 at 22:08
@Seth Carnegie: You just need to add a little check in case `*i` is a null pointer (in which case you would just break out of the loop) :) – someguy Jan 14 '12 at 22:10
@someguy why (besides efficiency)? – Seth Carnegie Jan 14 '12 at 22:10
@Seth Carnegie: because copying the vector may have failed half way, so the other half would be null pointers – someguy Jan 14 '12 at 22:11
@someguy yes but it would still work fine (although doing a little extra work) so it's not absolutely necessary. I will add it though since it would be more efficient. – Seth Carnegie Jan 14 '12 at 22:11
@Seth Carnegie: Deleting a null pointer results in undefined behaviour. Also, you don't need to set `*i` to NULL. – someguy Jan 14 '12 at 22:13
2

@someguy actually calling `delete` on a null pointer is defined to do nothing. Not sure why I set `*i` to null, seemed like a good idea at the time :) At least now I can get rid of the curly braces. – Seth Carnegie Jan 14 '12 at 22:15
@Seth Carnegie: Oh yeah you're right. I think I got it mixed up with dereferencing a null pointer :p. – someguy Jan 14 '12 at 22:21
@SethCarnegie: Looks like it handles the case I had in mind, yes. – Emil Styrke Jan 16 '12 at 06:12

score 0 · Answer 2 · answered May 09 '21 at 08:59

Ten years after C++11, you should be able to take advantage of its features. reserve() to preallocate enough room and still be able to push_back or emplace_back, though for plain pointers this doesn't matter. And range-based for loop to avoid explicit iterators

B::B(const B &orig) {
    data.reserve(orig.data.size());
    for (auto p : orig.data)
        data.push_back(new A(*p));
}

And finally, you may use std::unique_ptr to be safe and avoid memory leaks and the need to provide an explicit destructor

class B {
public:
    B();
    B(const B&);
protected:
    std::vector<std::unique_ptr<A> > data;
};

with the constructor changing to

B::B(const B &orig) {
    data.reserve(orig.data.size());
    for (auto &p : orig.data)
        data.emplace_back(new A(*p));
}

C++, how to correctly copy std::vector in copy constructor?

2 Answers2

Linked