Deleting a base object inside a derived class's destructor

[caut_baia]

Hi folks i only have one question.Can or should i delete a base class object instantiated in a derived class's constructor , inside the derived class's destructor?

class A {
protected:
   int somedata;
public:
   A () {}
   A (int x) : somedata(x) {}
   virtual ~A () {} 
 };
class B : public virtual A {
   A* y;
public:   
   B () {}
   B (int x) : A(x), y(new A()) {}
   ~B () {
      delete y; //i get an exception and if i throw an std::exception() i get 'no named exception'
                //doing this do i call the destructor of A twice?And if so .. how do i free the memory of y without calling delete on it?
    }
  };

Declaring A's destructor non virtual seems to work but what about the heap allocated memory?What am i missing?Thanks for the time

[Moschops]

delete is what you use to deallocate some memory you allocated using new. I don't see any such allocation.

[mike_2000_17]

You are doing it correctly. Except, you are not giving a value to the y pointer when you are creating the object of class B without parameters (i.e. in the default constructor). This would mean that your pointer y will have an uninitialized (and thus invalid) value and deleting that invalid pointer would raise an exception or run-time error of some kind (depending on OS and compiler settings).

This would be the "proper" way to do it:

class A {
protected:
   int somedata;
public:
   A () : somedata(0) {} //better never leave uninitialized data.
   A (int x) : somedata(x) {}
   virtual ~A () {} 
 };
class B : public virtual A { //note: you don't need virtual inheritance here!
   A* y;
public:   
   B () : y(NULL) {} //initialize to NULL if nothing better can be done.
   B (int x) : A(x), y(new A()) {} 
   ~B () {
     if(y) //verify that y is not NULL before deleting it.
       delete y; //this should be safe.
    }
  };

The virtual inheritance that you have is not really useful (at least not in this simple example), and it has some oddities to be aware of.

The easiest thing to do here is to use smart pointers. That just makes everything easier. You can find them in either TR1 (std::tr1 namespace) or the Boost smart pointer libraries (they have the same names and behaviour, except that "scoped_ptr" is called "unique_ptr" in TR1.. I think). A shared_ptr use in this case would look like this:

class A {
protected:
   int somedata;
public:
   A () {}
   A (int x) : somedata(x) {}
   virtual ~A () {} 
};
class B : public virtual A {
   boost::shared_ptr<A> y; //note the declaration of y as a shared_ptr to A
public:   
   B () {} //no need to initialize y, it will initialize to "not a pointer".
   B (int x) : A(x), y(new A()) {}
   //note: no need for a destructor, the default it enough. Smart pointers automatically delete their pointed-to object if it exists.
};

[mrnutty]

if(y) //verify that y is not NULL before deleting it.
       delete y; //this should be safe.

just note, deleting NULL is defined and does nothing.

[Moschops]

if(y) //verify that y is not NULL before deleting it.
       delete y; //this should be safe.

just note, deleting NULL is defined and does nothing. So that if statement isn't necessary.

Indeed. Furthermore, just because a pointer isn't NULL doesn't make it safe to delete, which is exactly the trouble in the OP. The comment stating "//this should be safe" in fact holds no such guarantee.

[mike_2000_17]

>>"//this should be safe" in fact holds no such guarantee.
It is made safe by the fact that the pointer is in the private scope of the class B and that all the constructors of B make the pointer either NULL or valid. So, I say "this should be safe" not because of the trivial if-statement before, but because of the addition of y(NULL) to the default constructor. I thought that was clear from my explanation, I guess it wasn't, thanks for pointing it out.

>>just note, deleting NULL is defined and does nothing. So that if statement isn't necessary.
Thanks for pointing that out, it has been a while since I have actually used raw pointers (or even an actual delete statement). Also worth pointing out, not all implementations are really standard when you get down to the details of, I developed a habit of checking for NULL on pointers because I had to use an old Borland compiler that caused a crash if a null-pointer was deleted.

[Moschops]

Edit: Spoonlicker disease!

[caut_baia]

Thanks a lot.I overlooked some things like initialization to zero since i'm trying to focus the point on the other problem.Not that i'm not thankful.I know i don't need virtual inheritance since there aren't multiple base classes involved but thanks for pointing it anyway.One more question though.How would you implement B's copy constructor?

B (const B& cpy) : A(cpy), y(cpy.y) {} //I can't come up with something better than this

Cause i did this and in main () and when i try :

B x(5);
B y(x); //i get an exception again

[mike_2000_17]

>>//I can't come up with something better than this
You need to implement deep-copy semantics. The default copy-constructor, which is exactly like the code you put, will perform a shallow-copy. This means it simply makes the pointer y point to the same object as the cpy.y pointer. You will get an exception because when the first object gets deleted, it also deletes its y pointer, and when you reach the destructor of the second object, it will again try to delete its y pointer which is now invalid. Using the shared_ptr will solve this problem automagically by implementing shared ownership semantics (i.e. all copies of an object share ownership of the object pointed to by their y pointer), the shared_ptr will only deleted the pointed-to object when all object of class B that have a pointer to that object have been deleted. However, if shared ownership is not what you want, i.e. you want each object of class B to have a unique y pointer pointing to a unique object for each B-object. Then you need deep-copy semantics, which means that a new object of class A is created for the new copy and that that object is copied... as follows:

B(const B& cpy) : A(cpy) {
    delete y; //you must delete the old y in order to not leave a memory leak.
    y = new A(*cpy.y); //create a new copy of *cpy.y.
  };

Now, if that y pointer is not actually pointing to an object of class A but to an object of a class derived from A, then it becomes a bit more tricky because you cannot just create a new A-object and copy the cpy.y object into it. What is typically done in this case is to use a virtual clone() function:

class A {
protected:
   int somedata;
   virtual A* clone() const { return new A(*this); }; //invoke the copy-constructor
public:
   A () : somedata(0) {}
   A (int x) : somedata(x) {}
   virtual ~A () {} 
 };
class B : public A { 
protected:
   A* y;
   virtual A* clone() const { return new B(*this); }; //override the function in all derived classes.
public:   
   B () : y(NULL) {} 
   B (int x) : A(x), y(new A()) {} 
   B (const B& cpy) : A(cpy) {
     delete y; //delete the old y pointer.
     y = cpy.y->clone(); //call clone function on the cpy.y to create a copy of the most derived-class object.
   };
   ~B () {
     delete y; //this should be safe.
   }
  };

Another option for the copy-constructor is to use move semantics, but that's another story.

[caut_baia]

Thank you very much mike.I've read about Clone functions at some point but it didn't hit me just before you mentioned it.But just one thing.. This means that when i'm copying the pointer of the copied object i'm only getting the address and not the memory allocated on that address.Why is this so i wonder.If the object from which to copy was not yet destroyed it's y member should point to an existing A() object allocated on the heap.I kinda get it .. but i still feel like i don't.

[mike_2000_17]

Consider this simple piece of code:

int main() {
  int a_variable = 42;
  int* a_ptr = &a_variable;
  int* a_ptr_cpy = a_ptr; 
};

Now, if I understand you correctly, you are puzzled by the fact that when you copy a pointer, it doesn't make a copy of the memory it points to. Although it could have been implemented that way, it is not. The reason it is not, is because a pointer could point to anything (could be an object allocated on the stack, it could be an object allocated on the heap, it could be the start of an array, it could be just some value that is not an address but was casted to a pointer type... etc. etc.). The compiler cannot make assumptions like "oh, this pointer clearly points to an object allocated on the heap and that object is not from some other derived class" and then decide to make that memory copy for you. It is your responsibility to handle this correctly based on what you know. A pointer is just a address value, nothing else (of course, the address value is the address of something meaningful, but for the compiler, a pointer is not much more than an integer that holds an address in memory).

[caut_baia]

In your example *a_ptr_copy will be 42.So i'm assigning a pointer to another pointer but the compiler knows the type of the object pointed to by a_ptr_copy since it was declared as int.It will therefore fetch the next 4 bytes on a 32 bit system or at least that's my assumption.If it was the case of an array it would make perfect sense since copying a pointer to an array won't give enough information on the size of it.I know you're right about deep copy semantics since in works your way.I should compile this so i can see the machine code generated but i don't think 'new' will expand directly to machine instructions but rather an instance of an object.I'm using borland 5.5 at the command line.Anyway i hope i haven't tired you with my questions.I can appreciate more if you have more to give though :).Thanks a lot mike

[mike_2000_17]

Ok, so after you have copied a pointer, the two copies will of course point to the same address, i.e. the same variable or object or array. That's the idea. So of course, in my little code snippet, *a_ptr_copy will evaluate to 42, because that is the value of a_variable. The purpose of that snippet was just to illustrate that there are situations in which you can have pointers to objects or variables or arrays that were not allocated from the heap and so the compiler cannot assume that.

And also, a pointer is a record of the address in memory of "something". When you copy a pointer, you could do it because you want to hold a second record of the address in memory of that same "something", or you could do it because you want to have a pointer to a new "something". The compiler has to behave consistently and cannot infer as to what you "meant to do" it can only do what you tell it to do. So, in this case, the compiler will always assume that when you copy a pointer, you are holding a second record of the address in memory of the same "something" as the original pointer had. When you think about it, it has to be that way, otherwise it would be a mess (note that there are programming languages that implement the inverse behaviour (copy a pointer = copy the memory).. and they are a mess).

[caut_baia]

#include <iostream>
#include <exception>

class A  {
protected:
   int x;
   int arr[10];   
public:
   A (int n) : x(n) {
      for (int q=0;q<10;q++)  {
         arr[q]=q;
       }
    }
   A () : x(0) {}
   virtual ~A () {} 
   A (const A& cpy)  {
      x=cpy.x;
      for (int q=0;q<10;q++)  {
         arr[q]=cpy.arr[q];
       }
    }
   A& operator= (const A& cpy)  {
      x=cpy.x;
      for (int q=0;q<10;q++)  {
         arr[q]=cpy.arr[q];
       }
      return *this;
    }
   
   friend std::ostream& operator<< (std::ostream& outp,const A& obj)  {
      for (int q=0;q<10;q++)  {
         outp << obj.arr[q] << "\t";
       }
      return outp << obj.x;
    }
 };

class B : public A  {
   A* y;
public:
   B() : y(NULL) {}
   B (int n) : A(n), y(new A(x))  {}
   B (const B& cpy) : A(cpy), y(cpy.y)  {} //copy constructor works fine
    
   ~B ()  {
      std::cout << "ok"; //first object gets destroyed and all
      try  {
         delete y;
         throw (std::exception());
       }
      catch (std::exception& exc)  {
         std::cout << exc.what() << "\n";
       }
      catch (...)  {
         std::cout << "nasty thing.." << "\n";
       }
    }
 };

int main ()  {
   B b(5);
   B o(b);
   std::cout << b << "\n";
   std::cout << o << "\n";
   //error when o gets out of scope
 }

I messed with a code a bit.I hope i'm not becoming annoying :)

[mike_2000_17]

Well, one things that is annoying is that the code you posted has the same error that my early post was addressing (which is that you are doing a shallow-copy when you should be doing a deep-copy). You do understand that this code will crash:

int main() {
  int* ptr1 = new int;
  int* ptr2 = ptr1;
  delete ptr1;
  delete ptr2; //CRASH!
};

In the above, since both ptr1 and ptr2 point to the same location, you cannot call delete on both of them because as soon as one delete is called, both pointers are invalid and trying to call delete on either one of them will cause a crash.

In your code, the problem is exactly the same. You cannot make a copy of a pointer such that you end-up with two pointers pointing to the same location and then calling delete on both, whichever gets deleted second will cause a crash.
So, as a basic solution, you should try this:

#include <iostream>
#include <exception>

class A  {
protected:
   int x;
   int arr[10];   
public:
   A (int n) : x(n) {
      for (int q=0;q<10;q++)  {
         arr[q]=q;
       }
    }
   A () : x(0) {}
   virtual ~A () {} 
   A (const A& cpy)  {
      x=cpy.x;
      for (int q=0;q<10;q++)  {
         arr[q]=cpy.arr[q];
       }
    }
   A& operator= (const A& cpy)  {
      x=cpy.x;
      for (int q=0;q<10;q++)  {
         arr[q]=cpy.arr[q];
       }
      return *this;
    }
   
   friend std::ostream& operator<< (std::ostream& outp,const A& obj)  {
      for (int q=0;q<10;q++)  {
         outp << obj.arr[q] << "\t";
       }
      return outp << obj.x;
    }
 };

class B : public A  {
   A* y;
public:
   B() : y(NULL) {}
   B (int n) : A(n), y(new A(x))  {}
   B (const B& cpy) : A(cpy), y( new A( *(cpy.y) ) )  {} //deep-copy!!!!!
    
   ~B ()  {
      std::cout << "ok"; //first object gets destroyed and all
      try  {
         delete y;
         throw (std::exception());
       }
      catch (std::exception& exc)  {
         std::cout << exc.what() << "\n";
       }
      catch (...)  {
         std::cout << "nasty thing.." << "\n";
       }
    }
 };

int main ()  {
   B b(5);
   B o(b);
   std::cout << b << "\n";
   std::cout << o << "\n";
   //no error!
 }

[caut_baia]

Thank you a lot.The fact that after the first deletion the second pointer becomes invalid didn't cross my mind.It is six in the morning :D.I hope i didn't upset you with my last post but now you got rid of me by solving my problem so i won't be bothering you .. until next time anyway:).Thanks again