Search code examples
c++design-patternsshared-ptr

Recommended pattern for parent child relationship ownership


I am new to c++ and am trying to understand reference counted memory. In the code below I have 2 methods for returning the vector of rectangles

vector<RectangleRef> &rectanglesRef() { return rects_; }
vector<RectangleRef> rectangles() { return rects_; }

What i am unsure about is if either of them creates a new vector on the stack for the caller?

What is the recommend pattern when you have ClassA that contains many ClassB's in a vector that you want to be able to expose? Do you do

void addRectangle(RectangleRef r) { rects_.push_back(r); }
void removeRectangle(RectangleRef r);
vector<RectangleRef> rectangles() { return rects_; }

or do you just give callers access to the internal structure and allow them to add/remove things at will?

If the Rectangle class was to keep a back pointer to the Test class that it belongs to, is using

typedef std::weak_ptr<Test> TestWeakRef;
...
TestWeakRef test_;

the correct idiom?

Thanks for any help.

The code:

#include <iostream>
#include <limits>
#include <vector>

using namespace std;

class Point {
public:
    Point() {
        x_ = 0.0;
        y_ = 0.0;
    }

    Point(double x, double y) {
        x_ = x;
        y_ = y;
    }

    double x() const { return x_; }
    double y() const { return y_; }

    void setX(double x) { x_ = x; }
    void setY(double y) { y_ = y; }

    void offset(double dx, double dy) {
        setX(x() + dx);
        setY(y() + dy);
    }

    bool operator == (Point const &point) {
        return x() == point.x() && y() == point.y();
    }

    void operator = (Point const &point) {
        setX(point.x());
        setY(point.y());
    }

private:
    double x_;
    double y_;
};


class Size {
public:
    Size() {
        width_ = 0.0;
        height_ = 0.0;
    }

    Size(double width, double height) {
        width_ = width;
        height_ = height;
    }

    double width() const { return width_; }
    double height() const { return height_; }
    double area() const { return width() * height(); }

    void setWidth(double width) { width_ = width; }
    void setHeight(double height) { height_ = height; }

private:
    double width_, height_;
};

class Rectangle {
public:
    Rectangle() {
        origin_ = Point();
        size_ = Size();
    }

    Rectangle(double x, double y, double width, double height) {
        origin_ = Point(x, y);
        size_ = Size(width, height);
    }

    Point origin() const { return origin_; }
    Size size() const { return size_; }

private:
    Point origin_;
    Size size_;
};

typedef std::shared_ptr<Rectangle> RectangleRef;
typedef std::weak_ptr<Rectangle> RectangleWeakRef;

class Test {
private:
    vector<RectangleRef> rects_;

public:
    Test() {
        rects_ = vector<RectangleRef>();

        for (int i = 0; i < 100; i++) {
            RectangleRef ptr = make_shared<Rectangle>(i*1.0, 0.0, 1.0, 1.0);
            rects_.push_back(ptr);
        }
    }

    vector<RectangleRef> &rectanglesRef() { return rects_; }
    vector<RectangleRef> rectangles() { return rects_; }
};

int main(int argc, const char * argv[]) {

    vector<RectangleRef> r;
    vector<RectangleRef> r1;

    if (true) {
        Test t = Test();

        r = t.rectangles();
        r1 = t.rectanglesRef();

        if (r1 == r) { cout << "they match\n"; }
    }

    // insert code here...
    //std::cout << r->origin().x() << "\n";
    return 0;
}

Solution

  • Drew Dormann nicely answered the first few questions. I'm just adding a few things.

    First, you should really only use shared_ptr if you actually need shared ownership semantics. Reference counting has overhead and if you do not correctly break up cyclic dependencies, it will cause memory leak, so you shouldn't do it unless you actually need it.

    In this code, there's no real reason why you need to store a vector<RectangleRef> instead of a simple vector<Rectangle>. The latter also has the benefit of better data locality, which is important for caching.

    Second, this

    typedef std::weak_ptr<Test> TestWeakRef;
    // ...
    TestWeakRef test_;
    

    is generally not a good idea, because it constrains the lifetime management policy of Test. That is, because a weak_ptr can only refer to an object whose lifetime is managed by a shared_ptr, you just made it impossible to do something like Test t; or unique_ptr<Test> t;. Do not do this unless you have a really, really good reason.

    If you want back pointers, just use a simple non-owning raw pointer, which works regardless of whether the Test object is on the stack or managed by a unique_ptr or a shared_ptr or some other custom smart pointer:

    Test * test_;
    

    If the lifetime of Rectangles stored in a Test can exceed the lifetime of the Test (which can only happen if you do not have exclusive ownership of them (e.g., storing shared_ptrs to them), which is usually unnecessary), your Test destructor may have to go through the Rectangles it stores and clear out the pointer to prevent dangling pointers. But the typical case is that what is stored with a Test object will go away when the object is destroyed, in which case this is unnecessary.