Error Within Overloaded Assignment Operator Function and Copy Constructor

Question

I am currently writing a program that creates a dynamically allocated circular array. For it, I have created a copy constructor and an assignment operator.

I am getting an error called "munmap_chunk(): invalid pointer", when I try to call my assignment operator for a second time. If I call it once, the error does not show. Am I writing my copy constructor and assignment operator properly? If there is any information needed I am happy to provide, thank you.

CircularDynamicArray(const CircularDynamicArray& source){
    cout << "copy constructor called" << endl;
    m_capacity = source.m_capacity;
    m_size = source.m_size;
    m_front = source.m_front;
    m_rear = source.m_rear;
    arr = new elmtype[source.m_capacity];
    for(int i = 0; i < source.m_capacity; i++) {
        arr[i] = source.arr[i];
    }
}
//overloaded assignment operator
CircularDynamicArray &operator = (const CircularDynamicArray& source) {
    cout << "Overloaded Assignment called" << endl; 
    //check for self assignment
    if (this == &source) {
        return *this;
    }
    m_capacity = source.m_capacity;
    m_size = source.m_size;
    m_front = source.m_front;
    m_rear = source.m_rear;    
    delete[]arr;
    for(int i = 0; i < source.m_capacity; i++) {
        arr[i] = source.arr[i];
    }
    return *this;
}

You are `delete[]`ing `arr` and then immediately you index into it with `arr[i]` in the following loop. This is causing undefined behavior. — walnut, Oct 12 '19 at 01:09
If you have written a destructor, your assignment operator need not be so complicated if you used "copy / swap". As a matter of fact, it would be trivial -- just 5 `std::swap` lines -- no allocating memory, no deleting memory, and everything would work. — PaulMcKenzie, Oct 12 '19 at 01:49

PaulMcKenzie · Accepted Answer · 2019-10-12T02:08:33.153

Am I writing my copy constructor and assignment operator properly?

I will say that you are making more work for yourself than necessary when it comes to writing an assignment operator.

If you have written a copy constructor (which you have), and a destructor (which you didn't show, but let's assume you did), and both of these functions have no bugs, then the assignment operator can be implemented trivially using copy / swap.

In general, you should strive to write the copy constructor and destructor before writing the assignment operator, so that this "trick" of writing the assignment operator can be utilized. Here is an example:

#include <algorithm>
//...
CircularDynamicArray &operator=(const CircularDynamicArray& source) 
{
  if (this != &source) 
  {
      CircularDynamicArray temp(source);    // Create a copy of what we want
      // get the temp's innards, and give temp our stuff
      std::swap(temp.m_capacity, m_capacity);
      std::swap(temp.m_size, m_size);
      std::swap(temp.m_front, m_front);
      std::swap(temp.m_rear, m_rear);
      std::swap(temp.arr, arr);
  }  // goodbye temp

  return *this; 
}

No allocating memory, no delete[] calls, you don't even need a check for self-assignment (but done anyway, for efficiency purposes). The above is exception-safe also. Everything you need for an assignment operator to work, basically flawlessly.

Note that you need to swap all of the member variables -- don't forget any, as that will cause this to not work correctly.

All that is being done is that you're making a copy of the passed-in object, and swapping out the guts of the current object this with the copy. Then the copy dies off with your old information. This is why you need a working copy constructor (for the initial copying of the source to work), and a working destructor (so that the destruction of the temp works).

Error Within Overloaded Assignment Operator Function and Copy Constructor

1 Answers1