0

So I'll try to be as clear as possible about this. Say we have the following:

class X : protected vector<objectY*> 
{
private:
objectZ** array;
unsigned size;
unsigned capacity;

public:
X():size(0), capacity(0), array(new objectZ*[100]){}
X(const objectX& o); // to be defined later 
X& operator=(const X& x); //defined later
~X(){ delete [] array; clear(); } <--- is this correct, or it produces memory leak?
void add(objectZ* o); // used to add objects to array

}

Now let's say we already defined class Y and Z with all the basic stuff we need for the above code to compile. My question is the following: Is the destructor correct? Does my code have memory leak?

Say I go to main() and I do this:

objectZ* o1 = new objectZ();
objectX* x1 = new objectX();

x1->add(o1);

 delete o1; // to avoid memory leak;
return 0; // end of main.

Since I'm mostly adding stuff to objectX's array from main, and most likely using polymorphism, like

 objectZ* ok = new objectK(); // where k is derived from Z
 x1->add(ok);

how do I define properly the destructor of class X without having memory leaks, and considering it also inherits from a vector of pointers to another objectY, do I need to cycle the vector 1 by 1 element and call delete on it, and clear() at the end or is clear() enough? Do i cycle array and call delete on it's elements too? My thoughts are that since I'm not allocating memory from within class X, I don't need to deallocate it either. I only need to delete [] array, and possibly clear() the inherited vector.

Roben
  • 1
  • 3
    `std::vector` is [not designed/intended to be derived from](https://stackoverflow.com/questions/4353203/) (none of the standard library containers are). And why are you not using a `std:vector` for the `objectZ*` array? – Remy Lebeau Jun 25 '19 at 22:17

1 Answers1

1

Is the destructor correct? Does my code have memory leak?

Possibly. If X is supposed to take ownership of the objectZ objects in its array, then it needs to free those objects before then freeing the array itself, otherwise they are leaked.

On a side note, your main() example is leaking the x1 object. It is not clear whether you meant to call delete x1 instead of delete o1, or whether x1->add(o1) is supposed to take ownership of o1 or not.

how do I define properly the destructor of class X without having memory leaks

By not doing manual memory management in the first place. Make proper use of smart pointers, std::unique_ptr and std::shared_ptr, and let them handle the memory management for you.

considering it also inherits from a vector of pointers to another objectY, do I need to cycle the vector 1 by 1 element and call delete on it

Quite likely, yes. It really depends on the ownership model that X employs. If X takes ownership of the objectY and objectZ objects, then it is responsible for freeing them. Otherwise, if it does not take ownership, then it is not responsible for freeing them.

and clear() at the end or is clear() enough?

For an array of raw pointers, clear() simply removes the pointers from the array, it does not free the objects being pointed at. An array of smart pointers, on the other hand, will free the objects when the smart pointers are removed from the array.

My thoughts are that since I'm not allocating memory from within class X, I don't need to deallocate it either.

Any object allocated with new must be freed with delete. Whether that needs to happen inside of X or not will depend on X's particular ownership model.

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • Why would you need to `clear()` before `delete[]`ing an unrelated pointer? In fact, the `clear()` is completely redundant anyway. – Konrad Rudolph Jun 25 '19 at 22:25
  • @KonradRudolph I reworded that part of my answer – Remy Lebeau Jun 25 '19 at 22:28
  • So does clear() call the destructor of all objectY pointers contained in the vector? Or I should do something like: for(int i=0; i – Roben Jun 25 '19 at 22:31
  • @Roben "*does clear() call the destructor of all objectY pointers contained in the vector?*" - No. I covered that in my answer. "*I should do something like: `for(int i=0; i` and `std::unique_ptr` instead of `objectY*` and `objectZ*`, respectively. – Remy Lebeau Jun 25 '19 at 22:33
  • My bad, saw the edited answer after commenting. Also, if by `x` taking ownership of `objectY` you mean I add objects to the vector using `new` as in: `push_back(new objectY*())` then no `x` doesnt take ownership of `objectY`, I simply `push_back(objectY* pointer)` which is passed as a parameter to the add function. Same thing for `array`. So no ownership is taken in that sense, do I still have to call delete on all objects contained in the vector, and all objects contained in `array` and use `clear()` and `delete [] array` respectively? – Roben Jun 25 '19 at 22:42
  • @Roben No, that is not what I mean by ownership. But in any case, if `X` DOES NOT take ownership of the objects added to its vector/array, then it MUST NOT call `delete` on them, only call `delete[]` on the array and `clear()` on the vector. On the other hand, if `X` DOES NOT take ownership, you must ensure that `X` is destroyed BEFORE the objects in its array/vector are destroyed, otherwise it will be holding dangling pointers to invalid memory. It would really help if you fix your question to provide better examples of what exactly you are trying to accomplish with `X` – Remy Lebeau Jun 25 '19 at 22:45
  • @Roben from C++11 onwards, storing of raw pointers implies observation only, not ownership. Storing of smart pointers implies ownership. – Remy Lebeau Jun 25 '19 at 22:49
  • Got it. So simply put, since I allocate memory in `main()`, using `new` to create the `Y` and `Z`, and store a pointer to those objects in `X`, `X object` does NOT own those pointers since they're raw pointers and thus it's not responsible for freeing the memory. So `~X(){ delete [] array; clear(); }` would be the correct implementation in this case. Also referring to what you said earlier, does `clear()` and `delete[] order `matter in unrelated pointers? Is `clear()` necessary or I can just do `delete[] array` and be done with the dtor? – Roben Jun 25 '19 at 22:58
  • @Roben You need to decide what your ownership model is. Do you want `X` to own the `objectY` and `objectZ` objects, or just point to objects that are owned by code outside of `X`? You have not said one way or the other what you actually want. As for `clear()`, it is not required in `X`'s destructor since the inherited `vector`'s destructor will be called after `~X()` exits, thus clearing the inherited `vector` for you. But I strongly suggest you replace your `objectZ*` array with a `std::vector` then you don't have to worry about calling `delete[]` on the array at all. – Remy Lebeau Jun 25 '19 at 23:10