Copy constructor of a class with a const vector member

Question

I have the class OpenTable in my code:

class OpenTable : public BaseAction {
public:
    OpenTable(int id, std::vector<Customer *> &customersList);
    OpenTable(const OpenTable& other);
private:
    const int tableId;
    const std::vector<Customer *> customers;
};

I need to implement a copy constructor for this class (I know it's probably a bad design, but I was instructed to do so). I ran into a problem when trying to deep copy the const vector customers.

I tried the following:

OpenTable::OpenTable(const OpenTable& other): tableId(other.tableId)
{
    for(std::vector<Customer*>::const_iterator i=other.customers.begin(); i!=other.customers.end(); i++) {
         customers.push_back((*i)->copy()); // copy returns a Customer*
    }

But clearly it doesn't compile, probably because the vector is const, thus I can't add elements to it.

I get the following error:

no instance of overloaded function "std::vector<_Tp, _Alloc>::push_back 
[with _Tp=Customer *, _Alloc=std::allocator<Customer *>]" matches the 
argument list and object (the object has type qualifiers that prevent a 
match) -- argument types are: (Customer *) -- object type is: const 
std::vector<Customer *, std::allocator<Customer *>>

Note: In the parameterized constructor I simply shallow copied because I could. This won't work for the copy constructor though.

Thanks in advance.

Is `Customer` a polymorphic base class? If not, why do you need pointers? And if you need pointers, have you thought about the *ownership* semantics and perhaps share the ownership of the `Customer` objects with a [`std::shared_ptr`](https://en.cppreference.com/w/cpp/memory/shared_ptr)? — Some programmer dude, Nov 08 '18 at 14:38
are the customers owned by the class? Are shared resources? Or just observing pointers. And the most important question: **What should the copy constructor do?** — bolov, Nov 08 '18 at 14:39
@FrançoisAndrieux The question you made this a dupe of doesn't contain an answer for when deep copying is needed to initialize the constant member. — Some programmer dude, Nov 08 '18 at 14:40
Create a helper function (which can be `private` and `static` member of your class if you want to limit access to it) that accepts a `const std::vector` and returns a `std::vector` that is a deep copy. Then, in the initialiser list of your constructor, use it to initialise your private member. — Peter, Nov 08 '18 at 14:41
@Someprogrammerdude This is a class derived from another (which is pure virtual), it was given to me as is (meaning I'm not allowed to delete members, only add new ones) — Barak B, Nov 08 '18 at 14:42
By the way, how do you do it with the other constructor? Do you make a deep copy of the vector *before* you construct the `OpenTable` object? — Some programmer dude, Nov 08 '18 at 14:44
@Peter It sounds like a good solution, however isn't it bad to call class methods from the constructor? Or is it ok because it's a static function (and belongs to the class rather than the object)? — Barak B, Nov 08 '18 at 14:45
@Someprogrammerdude Stated in the question, in the parametrized constructor I can shallow copy and it's working as intended. Tried to keep it short and not post the entire code here, sorry. — Barak B, Nov 08 '18 at 14:47
Anyway thanks all, @peter post the answer so I can mark it as the solution. — Barak B, Nov 08 '18 at 14:49
@FrançoisAndrieux You should remove the duplicate tag, it's misleading and incorrect. — Barak B, Nov 08 '18 at 14:50
@BarakB it's fine to call a non-virtual member that doesn't depend on (not-yet-initialised) member data. A `static` member easily meets those criteria — Caleth, Nov 08 '18 at 15:28
Yeah, okay. My previous comment isn't really a full answer, but I'll turn it into one shortly. — Peter, Nov 09 '18 at 09:16
As an aside, changing the pointers to `std::unique_ptr` will make it *so much easier* to provide exception guarantees for your class. — Caleth, Nov 09 '18 at 09:48

score 2 · Accepted Answer · answered Nov 09 '18 at 09:22

The simplest solution would be to create a helper function that accepts a const std::vector<Customer *> and returns a std::vector<Customer *> that is a deep copy of the argument. Then, in the initialiser list of your constructor, use that function to initialise your private member.

Optionally, that helper function can be a private and static member of your class if you want to limit access to it. That is acceptable in a constructor or its initialiser list, since a static member function does not rely on non-static members of class being initialised, so does not rely on the constructor having completed.

If a problem occurs in the helper function making the deep copy, the helper function will need to clean up appropriately (e.g. if construction of one of the Customer objects fails, any successfully constructed objects will need to be released to avoid a memory leak).

Caleth · Answer 2 · 2018-11-08T15:20:18.700

You are going to need something like boost::transform_iterator to do the copying as part of the member initialiser.

auto copy_iter(auto iter)
{
    return boost::make_transform_iterator(iter, [](Customer * c){ return c->copy(); });
}

OpenTable::OpenTable(const OpenTable& other)
 : tableId(other.tableId),
   customers(copy_iter(other.customers.begin()), copy_iter(other.customers.end()))
{}

or alternately

OpenTable::OpenTable(const OpenTable& other)
 : tableId(other.tableId),
   customers(boost::copy_range<std::vector<Customer *>>(other.customers | boost::adaptors::transformed([](Customer * c){ return c->copy(); })))
{}

score 0 · Answer 3 · answered Nov 08 '18 at 17:04

Why you use const class memebers? You can nothing do with them. Make the public access funtions read only instead.

std::vector<Customers *> const getCustumers() const { return customers;}
int const getTableId() const { return tableId; }

This will give you a read only vector of customers or a table id that can't be modifyed. Than the copy constuctor is no problem.

Copy constructor of a class with a const vector member

3 Answers3