C++ std::vector in constructor

Question

I am trying to code an effective implementation of the following composite class:

class composite{
  vector<base_class *> Vec;
  //Other useful constants
public:
  composite(vector<base_class*>);
  //Other useful operations...
};

My question is about the constructor and instantiation of the class and in particular the object Vec. At the minute, I use the rather crude implementation outlined below. I the implementation to be memory efficient. I'm pretty much a newb with C++, so I'm not sure I have the optimal solution here...

I use polymorphism to store different derived classes in a vector, e.g.

vector<base_class *> Vec1;
Vec1.reserve(2);
class1 * C1 = new class1(....);
Vec1.push_back(C1);
class2 * C2 = new class(....);
Vec1.push_back(C2);

where class1 and class2 are derived classes of base_class. I then pass Vec1 to the constructor of composite as follows:

composite::composite(vector<base_class*> Vec1){
   Vec.reserve(Vec1.size());
   Vec.swap(Vec1);
   //etc...
}

My feeling is that this is quite efficient on the memory, because Vec1 will be empty after the construction (it's elements have been swapped into Vec). On the other hand, it seems to be quite wasteful, as I am essentially copying the Vec1 into Vec. Is there a better way for me to do this? Can I somehow embed the vector Vec1 into composite? Thanks in advance!

`std::vector` of raw pointers is an utterly wrong idea. You want to use vector of unique or shared pointers, depending on what ownership those objects have. — Griwes, Jun 20 '13 at 10:00
Also take the vector by &, or && in the constructor, and make it explicit. — doctorlove, Jun 20 '13 at 10:02

Griwes · Accepted Answer · 2013-06-20T10:16:46.817

3

First, use proper smart pointer instead of raw pointer.

Next - in the method you used, the reserve() call is totally unnecessary - swap() just swaps internal pointers.

And last - since we're in 2013, C++11 is already to be used, so the constructor should look like this:

composite::composite(std::vector<std::unique_ptr<base_class>> v) 
    : vec{ std::move(v) }
{
}

Why this way? Taking the parameter by value already copies it, and since you aren't going to use that copy anymore, it is safe to be moved out, which achieves the least amount of copies to initialize the member.

edited Jun 20 '13 at 10:16

answered Jun 20 '13 at 10:10

Griwes

8,805
2
43
70

Can you move v if it's not an r-value reference? – doctorlove Jun 20 '13 at 10:11
1

@doctorlove, obviously, otherwise the move semantics would be totally useless... Let me improve the answer a bit. – Griwes Jun 20 '13 at 10:14
Why is taking a copy into the parameter better than offering a way to move it directly? (And thanks for correcting my nonsense earlier) – doctorlove Jun 20 '13 at 11:17
@doctorlove, because it works out of the box in every case. If the user will move something into this constructor, then it first will be moved out, then in into the value parameter, then out, then in into the member. But if the user wanted to pass an lvalue, then he would need to 1) make a copy of it 2) move it out. This method works for both those cases without boilerplate on caller's site. – Griwes Jun 20 '13 at 11:32

Torsten Robitzki · Answer 2 · 2013-06-25T06:59:27.407

If you really care about whether a copy of any vector will be made or not, you should first pass the constructors argument by reference. So the "usual" implementation would look like this:

composite::composite( const vector<base_class*>& Vec1 )
  : Vec( Vec1 )
{
}

This will omit already one copy. I wouldn't bother about this until you have some signs that this will cause any problem. You already did three dynamic memory allocations before, why do you care about a fourth?

C++ std::vector in constructor

2 Answers2

Linked