CODEWITHSUNDEEP
c++queuecopy-constructor
Mike
Mike

Reputation: 477

Copy Constructor not working for Circular Queue?

Im trying to make a copy of an object, which is a circular queue. My Enqueue and Dequeue work correctly, but whenever I do this, I get a runtime error.

CQUEUE j = k;

The output window says that my copy constructor is recursive on all control paths? Can someone help me figure out what I am doing wrong please? Here is my copy constructor, along with the overloaded assignment operator.

CQUEUE::CQUEUE(const CQUEUE& original)
{
(*this) = original;
}


void CQUEUE::operator=(CQUEUE w)
{
qnode *p = w.front;
(*this).front = new qnode;
(*this).back = front;

while(p -> next != w.back)
{
  back -> num = p -> num;
  p = p -> next;
  back -> next = new qnode;
  back = back -> next;
}

back -> num = p -> num;
p = p -> next;
back -> next = new qnode;
back = back -> next;
back -> num = p -> num;

back -> next = front;
front -> prev = back;
}

Upvotes: 0

Views: 1223

Answers (3)

Matthieu M.
Matthieu M.

Reputation: 299960

You're doing it backward, somewhat.

In general, the assignment operator is more complex than the copy constructor because it needs to properly dispose of the held resources on top of moving to the new value.

It is normally easier to compose a complex system out of simple parts rather than create a do-it-all part and express simple systems on top of it. In your context, it means than it is easier to create an assignment operator from a copy constructor.

Therefore, just write the copy constructor fully:

CQUEUE(CQUEUE const& c) { ... }

Then write a routine to swap (exchange the contents) of two CQUEUE instances:

void swap(CQUEUE& c) {
    using std::swap;
    swap(front, c.front);
    swap(back, c.back);
}

And finally, build the assignment operator out of them:

CQUEUE& operator=(CQUEUE c) { // copy
    this->swap(c);
    return *this;
}

This idiom is known as copy-and-swap (yes, very creative!).

Note: bonus note, if you have a move constructor, then it becomes a move-and-swap for free.

Upvotes: 1

Scotty
Scotty

Reputation: 2510

(*this) = original;

The compiler is making this line call the copy constructor instead of your = operator, because your = operator currently takes a non-const, by-value CQUEUE. Instead, your = operator should look like this:

void CQUEUE::operator=(const CQUEUE& w)

Then your copy constructor will call your operator = instead. Alternatively, make it more explicit:

CQUEUE::CQUEUE(const CQUEUE& original)
{
    Copy(original);
}

void CQUEUE::operator=(const CQUEUE& w)
{
    Copy(w);
}

void CQUEUE::Copy(const CQUEUE& other)
{
    // All your copy code here
}

EDIT: Yes, you're correct, you still do need a copy constructor.

Upvotes: 4

Jeeva
Jeeva

Reputation: 4663

Change the assignment operator function to and be careful about self-assignment

void CQUEUE::operator=(const CQUEUE& w)
{
   if (this == &w)
        return *this;

   //Paste your code
}

Upvotes: 1

Related Questions