C++: Runtime error when using overloaded compound assignment operators

Question

I have a program that uses a class to dynamically allocate an array. I have overloaded operators that perform operations on objects from that class.

When I test this program, the overloaded += works, but -= does not work. The program crashes when trying to run the overloaded -= and I get the following runtime error:

malloc: * error for object 0x7fd388500000: pointer being freed was not >allocated * set a breakpoint in malloc_error_break to debug

In the private member variables, I declare the array like this:

double* array_d;

I then dynamically allocate the array in an overloaded constructor:

Students::Students(int classLists)
{
    classL = classLists;
    array_d = new double[classL];
}

I have the following two overloaded constructors defined as friends of the Students class:

friend Student operator+= (const Student&, const Student&);
friend Student operator-= (const Student&, const Student&);

These are defined like so:

Student operator+= (const Student& stu1, const Student& stu2)
{
    if (stu1.getClassL() >= stu2.getClassL())
    {
        for (int count = 0; count < stu.getClassL(); count++)
            stu1.array_d[count] += stu2.array_d[count];

        return (stu1);
    }
    else if (stu1.getClassL() < stu2.getClassL())
    {
        for (int count = 0; count < stu1.getClassL(); count++)
                stu1.array_d[count] += stu2.array_d[count];

        return (stu1);
    }
}

Student operator-= (const Student& stu1, const Student& stu2)
{
    if (stu1.getClassL() >= stu2.getClassL())
    {
        for (int count = 0; count < stu2.getClassL(); count++)
            stu1.array_d[count] -= stu2.array_d[count];

        return (stu1);
    }
    else if (stu1.getClassL() < stu2.getClassL())
    {
        for (int count = 0; count < stu1.getClassL(); count++)
                stu1.array_d[count] -= stu2.array_d[count];

        return (stu1);
    }
}

Basically what is happening here is I am comparing two objects with arrays that differ in size based on classL. the getClassL() function is simply: int Student::getClassL() const {return classLists;}

In case you were wondering, I have overloaded the big three in this way:

1. Destructor: Student::~Student() {delete [] array_d;}

2. Copy Constructor:

Student::Student(const Student &student)
{
    classLists = student.classLists;
    array_d = student.array_d;
}

3. Assignment operator:

Student &Student::operator=(const Student &student)
{
    classLists = student.classLists;
    array_d = student.array_d;
}

It is weird that += works but -= does not work, since they are practically the same. I suspect my issue is in the allocation of my dynamic memory, but I am not sure and am seeking expert advice.

Answer 1

The advice to give you would be to use std::vector and spare yourself of having to implement assignment operator, copy constructor and destructor.

But having said that, there are a few things wrong in the code, let alone the copy constructor.

First, the copy constructor should be allocating a brand new array and copying the values from the array in the passed-in value to the new array. Here is a simplified version of your Student class with just two members -- a double * and an integer denoting the number of items in the array.

class Student
{
   int num;
   double *array_d;

   public:
      Student(const Student &student);
      Student& operator=(const Student &student);
      ~Student() { delete [] array_d; }
      Student() : array_d(0), num(0) {}
};

The copy constructor would look something like this:

Student::Student(const Student &student) : 
    array_d(new double[student.num]), num(student.num)
{
  for (int i = 0; i < num; ++i )
    array_d[i] = student.array_d[i];
}

Once you have this, the assignment operator is trivial using copy / swap:

Student& Student::operator=(const Student &student)
{
    Student temp = student;
    std::swap(d_array, temp.d_array);
    std::swap(num, temp.num);
    return *this;
}

Basically all the above does is make a temporary copy of the passed in object, exchanges the internals of the temporary object with the current object, and then the temporary dies with the old internals. All this cannot be possible unless the copy constructor and destructor for Student are working properly (which they should be now).

---

The next thing to consider is your whole idea for operator += and -=. The way most programmers expect to use this is in this context:

Student a;
Student b;
// assume a and b are initialized and have data...
a += b;

If you're using += or -= in a different form, it becomes unintuitive and just plain strange. Thus, those functions should be taking a single argument, not two arguments, and return the current object (it is the current object you're changing).

Therefore these functions should not be friend functions, but class member functions of Student:

class Student
{
   int num;
   double *array_d;

   public:
      Student(const Student &student);
      Student& operator=(const Student &student);
      ~Student() { delete [] array_d; }
      Student() : array_d(0), num(0) {}
      Student& operator += (const Student& rhs);
      Student& operator -= (const Student& rhs);
 };

Then the implementation would look something like this for +=:

#include <algorithm>
//...
Student& operator+= (const Student& stu1)
{
    int num_to_add = std::min(num, stu1.num);
    for (int count = 0; count < num_to_add; ++count)
        array_d[count] += stu1.array_d[count];
    return *this;
}

Similarly, -= would look like the above. Note the usage of std::min to determine how many to add, instead of your original code with an if / else.

Answer 2

Your question is more of a study about why C++11/C++14 is a better C++ than previous versions, and why one should use it as such.

First, as commentary has suggested, operators += and -= should return a reference, not a copy. If it were a member function operator, it would return *this, but in your case it's not. Just change the return type to Student &.

The reason for the crash, also covered in comments, is that when the copy constructor or assignment execute, the ownership of array_d is assumed by both 'Student' objects. When the second one is destroyed, it will attempt to delete [] array_d, which was already deleted by the first one.

The assignment operator shown returns nothing. It should return *this;

Commentary has pointed out you would be required to create a new array and copy each element from the source into the destination, so there are two separate arrays.

However, this is a key moment to illustrate why the STL is so valuable. If you had used std::vector<double> instead, vector's assignment operator would have copied all the elements for you.

In the two operator functions (both += and -=) the "else if" clause is of no value. Logically, if stu1.getClassL() is not >= stu2.getClassL(), it can only be <, so save the time and remove the else if clause, as well as the containing braces.