assignment operator overloading problem

Question

This issue has me confused. The first piece of code works fine without crashing, it assigns s1 to s2 perfectly fine. But the second group of code causes the program to crash.

Anyone have any idea on why this is happening or what the problem could be?

Code 1:(works)

    s1.add(10, 30, 25, "Test Screen", false);
s1.add(13, 10, 5, "Name:XXX", false);
s1.add(13, 18, 30);
s1.remove(-1);
Screen s2 = s1;

Code 2:(crashes on assignment)

    Screen s1;

    if (1 != s1.add(10, 30, 25, "Test Screen", false))
        message("first add() has a problem");
   else if (2 != s1.add(13, 10, 5, "Name:XXX", false))
        message("second add() has a problem");
    else if (3 != s1.add(13, 18, 30))
        message("third add() has a problem");
    else if (3 != s1.remove(-1))
       message("first remove() has a problem");
    else {
        Screen s2 = s1;
}

Assignment operator for screen class:

        Screen& operator=(const Screen &scr) {
        if (this != &scr){
            for (int i = 0; i < 50; i++) {
                if  (fields[i])
                    delete fields[i];
                fields[i] = new LField();
            }

            for (int i = 0; i < scr.numOfFields; i++)
                fields[i] = scr.fields[i];

            numOfFields = scr.numOfFields;
            currentField = scr.currentField;
        }
        return *this;
    }

Assignment operator for Field class:

LField& operator=(const LField &lfieldobj) {
        if (this != &lfieldobj) {

            if (lfieldobj.val) {
                if (val)
                    delete[] val;
                val = new char[strlen(lfieldobj.val) + 1];
                strcpy(val, lfieldobj.val);
            }
            else{
                //val = new char[1];
                val = "";
            }
            rowNum = lfieldobj.rowNum;
            colNum = lfieldobj.colNum;
            width = lfieldobj.width;
            canEdit = lfieldobj.canEdit;
            index = lfieldobj.index;

        }
        return *this;
    }

Any input would be greatly appreciated :)

Answer 1

What's the member "field" declaration?

LField* fields[50]?

If so, who's initializing the left hand side object fields member to NULL? I'd say that nobody... assignment operator is like copy constructor in C++, and you're invoking delete on a invalid pointer.

Answer 2

I've manged to fix it. It was a problem with memory allocation after all :)

Answer 3

Get rid of your current val and replace it with an std::string. Get rid of your fields and replace it with an std::vector. That should let you eliminate both of your overloaded assignment operators; the compiler will provide ones that work. I'd guess you'll eliminate the memory management problems along with the code.

As it stands right now, even if you "fix" the memory management problem(s) you know about, you're going to be left with the fact that your code is completely unsafe in the face of exceptions (and uses new so it basically can't avoid exceptions either).

Answer 4

        for (int i = 0; i < scr.numOfFields; i++)
            fields[i] = scr.fields[i];

That's not okay, you are copying a pointer instead of the pointed-to value. A deep copy is required.

Answer 5

The line

Screen s2 = s1;

actually invokes the Screen copy constructor, not the assignment operator overload.

For example:

#include <iostream>
using namespace std;

class Screen
{
public:
        Screen() { }

        Screen(const Screen& s)
        {
                cout << "in `Screen::Screen(const Screen&)`" << endl;
        }

        Screen& operator=(const Screen& s)
        {
                cout << "in `Screen::operator=(const Screen&)`" << endl;
                return *this;
        }
};

int main()
{
        Screen s1;
        Screen s2 = s1;
}

prints:

in Screen::Screen(const Screen&)

I'm guessing that your Screen copy constructor is defined similar to Screen::operator=(const Screen&), so a fix for the assignment operator overload may need to be applied to the copy constructor definition as well.

Also, how is the fields member of Screen defined? If it is like:

LField* fields[50];

then inside the constructors, you have to initialize all LField* objects in the array to NULL as they have undefined initial values:

std::fill_n(fields, 50, static_cast<LField*>(NULL));

Without this initialization, the test if (fields[i]) could succeed for some i even though fields[i] does not point to an allocation, causing your program to attempt to delete pointer(s) that were not returned by new.

Answer 6

For complicated objects it is better to use the copy and swap idum.
This gives you an assignment operator with the strong exception gurantee (transactionaly safe). But it also means that you only have to consider the complicated creation of the object in one place (the constructors).

Screen& Screen::operator=(Screen const& rhs)
{
    Screen tmp(rhs);
    this->swap(tmp);
    return *this;
}

void Screen::swap(Screen const& rhs) throw ()
{
     // Swap each of the members for this with rhs.
     // Use the same pattern for Field.
}