Setters for heap-allocated objects

Question

Suggesting there's a class that is composed of objects allocated in a heap.

class A
{
public:
    A() { m_b = new B; m_c = new C; }
    virtual ~A() { delete m_b; delete m_c; }

    inline B* b() const { return m_b; }
    inline C* c() const { return m_c; }

private:
    B* m_b;
    C* m_c;
}

Which setters would be the best for such code?

I figured out this one,

inline void setB(B* b) const { delete m_b; m_b = b; }
inline void setC(C* c) const { delete m_c; m_c = c; }

But there's a problem. If we added a non-heap variable or just a variable we did not have to delete with this setter, it would be deleted after the next invocation of the setter and this situation would cause an error or an unexpected behavior.

We can't delete objects directly as well because getters have const modifier. In addition, it would be not safe because an user of the class may not know whether the inner objects are allocated in a heap or not.

Could you please explain me how to use setters with heap-allocated objects?

No differently than with any other objects. It is your responsibility to manage objects to avoid memory leaks and undefined behavior. Unfortunately, there is no magic button somewhere one can push, and make this work right all the time. There is no universal law that tells you "how to use setters with heap-allocated objects". This is something you need to figure out by yourself. — Sam Varshavchik, Dec 20 '18 at 12:08
This is some massively confused ownership semantics. Work out who should be owning what, and the rest follows. — T.C., Dec 20 '18 at 12:09
check this: https://stackoverflow.com/questions/30615500/better-practice-for-heap-object-getters-setters-in-c — Dorel Tudor Ciucas, Dec 20 '18 at 12:10
Don't write classes that rely on having heap allocated objects. There's very very few good reasons for writing classes like that, but lots of bad reasons. If you explain why you feel the need for such a class then maybe someone can explain a better way to design your code. — john, Dec 20 '18 at 12:16
1: Don't use the term "heap" allocated objects its not useful in C++ (C++ is not Java). There are four types of objects (automatic/static/dynamic/thread) storage duration. Also ``*`` does not necessarily mean dynamically allocated. 2: Get/Set break encapsulation prefer to find a better way; usually `action` based methods. 3: Don't return pointers like that it does not define the ownership semantics off the object (thus leaks or bad deletes). 4: I assume you did not adhere to the rule of 3/5 because of the short example nature of the question. — Martin York, Dec 20 '18 at 12:25

score 4 · Accepted Answer · edited Dec 20 '18 at 12:09

4

Don't use raw new and delete. Use smart pointers instead - they're safer and easier to use. In your case, std::unique_ptr looks like a good candidate:

class A
{
public:
    A() : m_b{std::make_unique<B>()}, m_c{std::make_unique<C>()} { }
    virtual ~A() = default;

    inline B* b() const { return m_b.get(); }
    inline C* c() const { return m_c.get(); }

private:
    std::unique_ptr<B> m_b;
    std::unique_ptr<C> m_c;
}

Your setters can then simply be:

void setB(std::unique_ptr<B>&& b) { m_b = std::move(b); }
void setC(std::unique_ptr<C>&& c) { m_c = std::move(c); }

edited Dec 20 '18 at 12:09

T.C.

133,968
17
288
421

answered Dec 20 '18 at 12:09

Vittorio Romeo

90,666
33
258
416

Can we at least suggest that `geters` should return references. Or point out the danger of returning pointers that could be deleted. You may also want to note that this changes the original class as it is no longer copyable (though that does fix a potential bug). – Martin York Dec 20 '18 at 12:29
@MartinYork: I do not know what he is doing with the getters. Maybe he needs raw pointers? I tried to keep this as close as possible to the OP. I don't understand the second sentence in your comment. – Vittorio Romeo Dec 20 '18 at 12:30
He can always get a pointer from a reference by taking its address. Also not asking you to change the code just to make a note that its a bad idea. – Martin York Dec 20 '18 at 12:31
1

@MartinYork a programmer can delete a reference just as well as a pointer (after applying addressof operator). Problem with returning a reference is that there is potential for UB if when the pointer is null. – eerorika Dec 20 '18 at 12:53
They could. But by returning a reference you indicate you are not returning ownership (thus deleting is not your responsibility thus unlikely). Returning a pointer implies unknown semantics on who the owner is and thus very likely leads to a bug because of deletion. – Martin York Dec 20 '18 at 13:13
@MartinYork: returning a raw pointer in Modern C++ implies "no ownership". To imply ownership, we can use `gsl::owner` or a smart pointer. – Vittorio Romeo Dec 20 '18 at 13:17

Yakk - Adam Nevraumont · Answer 2 · 2018-12-20T12:20:22.023

Follow rule of 0.

class A {
public:
  A()=default;
  virtual ~A() = default;

  B* b() const { return m_b.get(); }
  C* c() const { return m_c.get(); }

  void set_b(std::unique_ptr<B> in){m_b=std::move(in);}
  void set_c(std::unique_ptr<C> in){m_c=std::move(in);}
private:
  std::unique_ptr<B> m_b=std::make_unique<B>();
  std::unique_ptr<C> m_c=std::make_unique<C>();
};

The unique ptr both makes ownership clear, fixes bugs you didn't know you had, and makes your code less fragile.

Bugs fixed:

In your original code, if new C threw, you'd leak a B.
If you copy or move the original objects they double-delete

If you want to mix owned and unowned subobjects, the first thing is don't do it. The second is to use fancier smart pointers. But types should have clear ownership semantics, and "maybe own maybe not" is code smell and will cause bugs.

Setters for heap-allocated objects

2 Answers2