-1

here is a simple example of my code

class base
{ 
    protected:
        int value; 
    public:
        base();
        base(const int);
        base(const base &);
        ~base();
];
class derived:public base
{
   protected:
        derived * next; // a pointer to the same datatype.
   public:
        derived();
        derived(const int);
        derived(const base &);
        derived(const derived &);// This is the problem.
        ~derived();
        ... ...
};

My problem is how to write the copy constructor of the derived class, because there is a pointer to derived class as a data member inside the derived class. Do I need deep copy to copy this pointer "derived * next" ? How about "next -> next" or "next -> next > next"? It looks like a infinite loop.

Here is what I write so far

derived::derived(const derived & source):base(source)
{
    if(next)
        delete next;
    next = new derived(source.next);
}
642720452
  • 7
  • 2
  • 5
    `derived & next;` is not a pointer, it's a reference. You're also missing a virtual destructor. – chris Jul 05 '14 at 21:10
  • 5
    You can deduce everything you need to know from the body of your destructors. The destructors tell you what class invariants are expected. Corollarily. without seeing the destructors we cannot possibly know the answer. – Kerrek SB Jul 05 '14 at 21:13
  • 4
    You get to define the semantics of your class. What *should* copying it do? – Alan Stokes Jul 05 '14 at 21:15
  • `new derived(source.next)` is passing a pointer instead of a reference. You should also define a stop condition.` – Frederik.L Jul 05 '14 at 21:33
  • Regardless of the semantics you want for the copy, I think there is a problem in your `delete next` because you will delete the object the source is still pointing to. – Christophe Jul 05 '14 at 23:44
  • You don't need to test 'next' for null before deleting it. The delete operator already does that. No need to double the checking. – user207421 Jul 06 '14 at 01:31

2 Answers2

1

I think the two misconceptions here are the incoherent parameter passed in the constructor and the absence of stop condition in the recursive chain. It will likely go for an infinite loop since there's no reason to stop. Something like this should do it, assuming that the structure isn't circular:

derived::derived(const derived & source):base(source)
{
    if(source.next) // if next is NULL, stop copying
    {
        // get the object that is pointed instead of pointer itself
        next = new derived(*source.next);
    }
}

UPDATE :

As it is suggested, checking next member inside the constructor is quite useless. In fact, the "anti-leak" security must be done at the assignation level, where the actual pointer value can be lost. Still assuming that the structure is a good candidate for recursion, you could be safe by doing this:

derived& derived::operator=(const derived & source)
{
   if (this == &source) return *this; // copy of itself is already finish

   freeMem(*this); // ensuring that the structure is empty prior to copy

   if (source.next)
      this->next = new derived(*source.next); // re-use the copy ctor

   return *this;
}

given this possibility for freeMem:

void derived::freeMem(const derived & source)
{
   if (source.next)
   {
      freeMem(*source.next); // this will force to delete from the end
      delete source.next;
      source.next = NULL;
   }
}
Frederik.L
  • 5,522
  • 2
  • 29
  • 41
  • 1
    This is a constructor, how would a prior value get into `next`? – Ben Voigt Jul 05 '14 at 21:50
  • The copy parameter may already have a `next` value, which may also already have one. OP's code is showing an attempt to do a deep copy of the whole structure recursively. – Frederik.L Jul 05 '14 at 21:53
  • Oh yes, testing `if (source.next)` makes perfect sense. But what can you learn from testing `if (next)`... it's uninitialized! – Ben Voigt Jul 05 '14 at 21:54
  • Only for security purpose. Modifying a pointer value => you don't want to let anything in it because you can't go back there. – Frederik.L Jul 05 '14 at 21:59
  • 1
    Under what possible conditions could `delete next;` possibly be correct inside the constructor of the type `next` is a member of? – Ben Voigt Jul 05 '14 at 22:01
  • The first element of the chain may already have an initialized `next` member, that is the point of this security. Also, this is a good practice to delete any possible value before loosing its location. The condition, though, could be into a `setNext` method to make sure that this member cannot leak memory. – Frederik.L Jul 05 '14 at 22:19
  • 1
    `next` is an uninitialized pointer. You say it could have an initialized `next` member, I ask you how? If it has a non-null value, it's a wild pointer. No good can come of `delete next;` here in the constructor. – Ben Voigt Jul 05 '14 at 23:06
  • I must agree with you on that, this is quite useless in a constructor. What I had in mind at the moment of answering the question is that the object that was about to be copied could still have an existing `next` attribute, although the leak potential is at another level in the end. Will update my answer on that aspect, thanks for pointing this :) – Frederik.L Jul 06 '14 at 17:36
0

If next is meant to represent a non looping chain (aka list) of derived, your code could be: 

derived::derived(const derived & source) :base(source.value)
{
    if (source.next) {   // if the source has a next pointer
        //delete next;   // don't delete ! 
        next = new derived(*source.next); // duplicate the object it contains 
    }
}

If the source has a valid next pointer this would duplicate all the following objects in the chain until the chain ends (with a null). All the source objects would remain unchanged (copy of all dependent objects).

This makes sense only if you also create an operator= to avoid that pointer are overwritten by an inadvertant = . Keep in mind that you have to avoid assigning the object on itself.

Christophe
  • 68,716
  • 7
  • 72
  • 138