1

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!

2
  • 3
    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, 2013 at 10:00
  • Also take the vector by &, or && in the constructor, and make it explicit.
    – doctorlove
    Jun 20, 2013 at 10:02

2 Answers 2

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.

4
  • Can you move v if it's not an r-value reference?
    – doctorlove
    Jun 20, 2013 at 10:11
  • 1
    @doctorlove, obviously, otherwise the move semantics would be totally useless... Let me improve the answer a bit.
    – Griwes
    Jun 20, 2013 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, 2013 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, 2013 at 11:32
1

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?

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.