2

I have a class A whose objects store a pointer.

This class's objects will either be used in another class (B)'s constructor which will take ownership of that pointer (thus making the A object useless) or be told that their pointer is useless and that they aren't needed anymore (which means they will free the memory pointed to by the pointer they hold).

So these objects have a pointer and a free method which deletes that pointer.

My question is, since the objects won't be needed anymore after they're either used by B's constructor or their method free is used, is it considered bad design / practice to tell B's constructor to also delete the A object it used, and to insert a delete this instruction at the end of A::free()? Or should I stick to manually delete those?

A little example:

A* myobj = new A(new Value(12));
B* myfinalobj;

if (is_useful(myobj)) {
    myfinalobj = new B(myobj);
    delete myobj;
}
else {
    myobj->free();
    delete myobj;
}

VS

A* myobj = new A(new Value(12));
B* myfinalobj;

if (is_useful(myobj)) {
    myfinalobj = new B(myobj); //myobj deletion is done by B::B(A*)
}
else {
    myobj->free(); //myobj deletion is done internally by A::free()
}
user6245072
  • 2,051
  • 21
  • 34
  • 7
    Using `new` and `delete` yourself is _considered bad practise_. Use smart pointers instead. – πάντα ῥεῖ Oct 17 '16 at 14:33
  • A little note on how I am using A objects: I am storing a large quantity of them in a `std::list` in a function, then returning that list. That list will be used in another function that will process it and decide if each element of the list is useful or useless, creating creating a new list of pointers to B. So no, deletion isn't happening next to allocation in this case. – user6245072 Oct 17 '16 at 14:37
  • @user6245072 still, you should use `std::unique_ptr`s for that. – Quentin Oct 17 '16 at 15:02
  • @Quentin for the pointers stored in the lists, or for the pointers stored in A objects? – user6245072 Oct 17 '16 at 15:07
  • 1
    @user6245072 you can start by replacing `new` with `std::make_unique`. Then, at every pointer assignment that now fails to compile, ask yourself who should be the owner of that object. The owner keeps the `std::unique_ptr` (via `std::move`), the others only get a raw, non-owning pointer (via `std::unique_ptr::get`). If you want to kill the object, either drop the `std::unique_ptr` or `reset()` it. – Quentin Oct 17 '16 at 15:17

3 Answers3

3

In general you should allocate the objects "close" to where you deallocate them.

So when you only use it like in your example you should keep the "delete" close and visible to the "new". There may be reasons where you are not able to do this and you have to comment this clearly when you take ownership.

Same as when you allocate objects in a constructor, delete them in the same class etc.

Howerver:

Since you tagged this c++11. There are smart pointers who display your intention and help you doing this.

If you only want one instance to have "ownership" you can use a std::unique_ptr<A> and move it into the the class B. It will then delete the instance of A automatically when B gets destructed.

If you have shared ownership, you can use std::shared_ptr<A>.

This will delete the instance of A when nobody has a reference to the pointer anymore.

If you can prefer unique_ptr to shared_ptr though.

Hayt
  • 5,210
  • 30
  • 37
  • See my comment to the question. – user6245072 Oct 17 '16 at 14:39
  • @user6245072 as I said "should" and there are reasons this cannot be. That is why with c++11 the smart-pointers got introduced. When you use those you don't have to worry about deallocation anymore. It is considered bad design to split up allocation and deallocation too much. In c++11 it is considered bad design to use new/delete in general. (also if you have no reason to use a list, use a `std::vector` they are almost always better) – Hayt Oct 17 '16 at 14:44
  • remember [this question](http://stackoverflow.com/q/39959231/6245072)? I accepted an answer, just to later realized it was unusable for me, so I ended up using void pointers again, which is why I can't use smart pointers. – user6245072 Oct 17 '16 at 14:48
  • @user6245072 ah that one. Yeah this will be one of those egde cases where you either have to redesign (use a common base class for wrapper) or have to be content with doing it this way and document everything clearly so people know these objects take ownership. "Bad practice" is nothing forbidden. It is just things that caused a lot of people headaches when trying to find the errors and may be error prone because of "forgetting" something. I saw on another script parser that types were wrapped in their own classes which had a common base class. But that would more fit your other question – Hayt Oct 17 '16 at 14:57
  • I'm going for the explicit `delete` then (1st example). – user6245072 Oct 17 '16 at 15:01
1

It's perfectly OK for an object to delete itself, see Is delete this allowed?

The bigger question is whether it's a good idea. In this case, your main code is creating a new A object with new. You would expect to see a matching delete to go with it, and hiding it within the object itself is a code smell.

Community
  • 1
  • 1
Mark Ransom
  • 299,747
  • 42
  • 398
  • 622
0

Does B need to work on memory previously allocated by A ?

First method

The first example seems to be be the best one I think, you can rely on the destructor of A to free internal memory allocation

Second method

Use smart pointers form STL, boost, or implement your own

Third method

Use RAII. The destructor of objects will be called at the end of the frame (closing bracket at the same nesting level ).

Dali
  • 344
  • 1
  • 10
  • But the destructor of A can't delete that pointer's object on its own, or when B takes ownership of that pointer and A gets deleted, B will have a dangling pointer. That's the whole purpose of `A::free()`, to only get called when the pointer isn't used by anyone else. – user6245072 Oct 17 '16 at 14:43
  • Don't try to manage the A's memory in the program flow, this is not the purpose of your code. You can set a boolean in A to know if memory need to be released, or you can copy the memory handled by a in B when needed, so you can always delete it in A's destructor – Dali Oct 17 '16 at 14:45
  • I don't think I understand that last comment. – user6245072 Oct 17 '16 at 14:46
  • By example implement A::GetList() method to copy your list in B. You could call it in B's constructor, then the list is duplicated and you can delete A entirely – Dali Oct 17 '16 at 14:49