CODEWITHSUNDEEP
c++polymorphismsmart-pointersstdvector
Emmanuel Lozoya
Emmanuel Lozoya

Reputation: 136

C++ Smart Pointers in a Vector container

I wrote the classic Shape Polymorphism code, but a little bit different because I'm using a Vector container and smart pointers.

I am not a C++ expert, and I would like to know the following:

  1. Is there any memory leak?
  2. If I don't call shapes.clear(), is there going to be a memory leak?
  3. Is there a better way to use smart pointers and containers?

Code:

#include <iostream>
#include <memory>
#include <vector>
using namespace std;

class Shape {
  public:
    virtual float getArea() = 0;
    virtual ~Shape() {}
};

class Rectangle : public Shape {
  public:
    Rectangle(float w, float h) : width(w), height(h) {}

    float getArea() {
      return width * height;
    }
  private:
    float width;
    float height;
};

class Circle : public Shape {
  public:
    Circle(float r) : radius(r) {}

    float getArea() {
      return 3.141592653589793238462643383279502884 * radius * radius;
    }
  private:
    float radius;
};

void printShapeAreas(vector<shared_ptr<Shape>> &shapes) {
  for(int i = 0; i < shapes.size(); i++) {
    cout << shapes[i]->getArea() << endl;
  }
}

int main(int argc, char** argv) {
  vector<shared_ptr<Shape>> shapes;
  //this works, but you told me that is better to use make_shared
  //=============================================================
  //shapes.push_back(shared_ptr<Shape>(new Rectangle(10, 2)));
  //shapes.push_back(shared_ptr<Shape>(new Rectangle(10, 3)));
  //shapes.push_back(shared_ptr<Shape>(new Circle(2)));
  //shapes.push_back(shared_ptr<Shape>(new Circle(3)));
  //better
  //======
  shapes.push_back(std::make_shared<Rectangle>(10, 2));
  shapes.push_back(std::make_shared<Rectangle>(10, 3));
  shapes.push_back(std::make_shared<Circle>(2));
  shapes.push_back(std::make_shared<Circle>(3));
  printShapeAreas(shapes);
  shapes.clear();//If I don't call shapes.clear(), is there going to be a memory leak?
  return 0;
}

Thank you :)

Upvotes: 6

Views: 9397

Answers (6)

milleniumbug
milleniumbug

Reputation: 15814

  1. There isn't, not in the code you posted - even without virtual destructor, std::shared_ptr destroys the elements properly. (you should add it anyway though - sooner or later someone will hit this issue, as std::unique_ptr won't handle this case)
  2. There won't be any memory leak, even if you don't call .clear() - the destructor of std::vector destroys all the elements, and the shared pointers destroys the element owned if they're the only owner.
  3. If you don't need shared ownership, you can use std::unique_ptr instead, but only if you have a virtual destructor in Shape. You should also use std::make_shared and std::make_unique (C++14) wherever you can to avoid a tricky issue with sequence points and throwing constructors: in essence f(std::unique_ptr<ConstructorAlwaysThrows>(new ConstructorAlwaysThrows()), std::unique_ptr<Foo>(new Foo())); will leak memory, or not, depending on how the compiler does sequencing.

Upvotes: 3

Guvante
Guvante

Reputation: 19203

Is there any memory leak?

No, you are calling the destructor of shared_ptr here.

If I don't call shapes.clear(), is there going to be a memory leak?

It depends, if your vector lives then all of its contents will live, if the vector dies then it will kill its contents automatically.

Is there a better way to use smart pointers and containers?

make_shared can be used to avoid problems if the new throws an exception by ensuring that the shared_ptr calls its destructor to delete the memory. However that would require a conversion from shared_ptr<Rectangle> to shared_ptr<Shape> which is automatic if annoying (it involves copying the shared_ptr and deleting the original).

Upvotes: 1

Christophe
Christophe

Reputation: 73376

1) I don't see a memory leak here. However there is a potential to getting one sooner or later: your shape destructor is not virtual. This means that shapes are always destroyed using the base destructor, instead of the right destructor. I.e. if one of your derived shapes would one day allocate memory, it's destructor that would have to release it wouldn't be called.

2) Nothing will happen: if you don't do a clear(), shapes will get destroyed anyway when main() is left, causing its contend to get destroyed.

3) Maybe, but this one is already very good.

Upvotes: 6

user2209008
user2209008

Reputation:

No, there won't be a memory leak, which is (part of) the entire point of smart pointers. When the vector falls out of scope, it calls the destructor on all of its elements. In this case, calling the destructor on the shared_ptr<Shape> will cause it's internal shared reference count to hit 0 and will thus destroy the Shape object.

Upvotes: 3

Fatih BAKIR
Fatih BAKIR

Reputation: 4715

Your code doesn't contain a memory leak. So your first point is fine.

No, if you don't call shapes.clear(), there will not be a memory leak since std::vector's destructor cleans up the container.

I don't know a specific rule about using shared pointers in a container so I'll skip on your third question.

However, I can offer an improvement for creating std::shared_ptrs:

When you create a shared pointer using its constructor, ie shared_ptr<T>(new T());, the control block or the structure that holds bookkeeping information about that resource, is created separately from the object it points to. This might cause cache misses a lot. However, if you create a shared_ptr by using std::make_shared, the control block is allocated with the object you want, therefore, by keeping them together, you can at least mitigate the cost of cache misses: std::make_shared<T>();. For example: std::make_shared<Circle>(3) is equivalent to writing std::shared_ptr<Shape>(new Circle(3)), but usually better.

Upvotes: 9

Edward Strange
Edward Strange

Reputation: 40859

  1. No.
  2. No.
  3. Maybe. Depends on your needs. What you've done is reasonable for the program you pasted.

Upvotes: 1

Related Questions