Access Violation Writing location 0x00000028

Question

I'm having some issues with the Assignment Operator. Although there are no red underlining errors, when I compile the program would break at

emp1 = emp2;

in main.cpp, everything was working until I added the assignment operation function.

Any Advice will be greatly appreciated.

Edit: Just thought I should show specific code that relates to the issue instead of the whole thing.

Here's what I've written:

ListOfEmp.h

public:
    ListOfEmp();
    ListOfEmp(const ListOfEmp &);
    ~ListOfEmp();
    const ListOfEmp& operator=(const ListOfEmp e);

};

ListOfEmp.cpp

ListOfEmp::ListOfEmp():head(NULL)
{
}

ListOfEmp::ListOfEmp(const ListOfEmp &e) {
    *this = e;
}

ListOfEmp::~ListOfEmp()
{
    clear();
}


const ListOfEmp& ListOfEmp::operator=(const ListOfEmp e){
    if (this != &e) {
        clear();
        EmpNode* copy = NULL;
        EmpNode* orig = e.head;
        while (orig != NULL) {
            if (head = NULL) {
                head = copy = new EmpNode((orig->emp).name, (orig->emp).salary);
            }
            else {
                copy->next = new EmpNode((orig->emp).name, (orig->emp).salary);
                copy = copy->next;
            }

            orig = orig->next;
        }
    }
    return *this;
}

void ListOfEmp::clear() {
    EmpNode* temp = head;
    while (temp != NULL) {
        temp = temp->next;
        delete head;
        head = temp;
    }
}

Main.cpp

int main() {
    ListOfEmp emp1;
    emp1.insertAtfront("John", 20000.00);
    emp1.insertAtfront("Dave", 24500.50);
    emp1.insertAtfront("Joshua", 33567.60);
    emp1.deleteMostRecent();
    emp1.getSalary("Dave");
    cout << endl;
    cout << emp1;
    cout << endl;

    ListOfEmp emp2;
    emp2.insertAtfront("Julio", 54000.00);
    emp2.insertAtfront("Mike", 12000.00);
    emp2.getSalary("Mike");
    cout << endl;
    cout << emp2;
    cout << endl;
    emp1 = emp2;
    cout << emp1;
    cout << endl;
    cout << emp2;
    system("pause");
}

`*this = e;` in the copy constructor smells 100 miles against the wind. — user0042, Nov 22 '17 at 00:50
Could you explain? I thought this was what I should've written for the copy constuctor. — Kujinn, Nov 22 '17 at 00:55
Depends on how good your `operator=` is. The conventional wisdom is to do it the other way around, with `=` based on the copy constructor. Reading on that: [What is the copy-and-swap idiom?](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) Not that this is sometimes a bit heavy, but it always works and that is worth it's weight in gold. — user4581301, Nov 22 '17 at 01:00
As for `operator=`, `operator=(const ListOfEmp e)` is pass by value, so it invokes the copy constructor which invokes `operator=` which invokes the copy constructor which invokes `operator=` which invokes the copy constructor which invokes `operator=` ... Get the problem? — user4581301, Nov 22 '17 at 01:02
`if (head = NULL) {` might also be a problem... Also remember that since they're currently calling the constructor, `head` hasn't been initialized when you call `clear()`, which will cause even more problems! — scohe001, Nov 22 '17 at 01:04
Turns out it was a visual studio issue that caused the error and not the code itself. That and the other things you guys pointed out. Everything works perfectly now, Thanks for the help, very much appreciated! — Kujinn, Nov 22 '17 at 11:56

user4581301 · Answer 1 · 2017-11-22T01:14:12.167

ListOfEmp::ListOfEmp(const ListOfEmp &e) {
    *this = e;
}

Bases the copy constructor around the assignment operator. Unfortunately, the assignment operator

const ListOfEmp& ListOfEmp::operator=(const ListOfEmp e){
    ...
}

takes the ListOfEmp to be assigned by value, invoking the copy constructor which invokes the assignment operator which invokes the copy constructor which invokes the assignment operator which invokes the copy constructor which invokes the assignment operator which invokes the copy constructor....

Uncontrolled recursion.

The solutions are pass by reference

const ListOfEmp& ListOfEmp::operator=(const ListOfEmp & e){
    ...
}

and rewriting the other way around, Assignment operator based on the copy constructor, to take advantage of the ever-popular Copy and Swap Idiom.

If you go with the first option, note the assignment operator is overly complicated and has a logic error

if (head = NULL) // Whoops. Annihilated head! Should be head == NULL

and the incomplete code leaves plenty of room for other errors in code that has not been provided.

scohe001 also correctly notes that head is not being initialized in the copy constructor. This is more likely than not the mistake that is triggering the crash.

While going with the first option and passing by reference, as well as fixing the (head = NULL) problem, I'm still getting the same but slightly different error. Maybe the method I used is inefficient compared to the Copy and Swap Idiom linked. I will try rewrite the code and see if that helps solve this issue. — Kujinn, Nov 22 '17 at 01:31
@Kujinn Two types of efficient at war here. Copy and swap actually tends to run a bit slower, so in that way it is less efficient. However, Its great glory is in being harder to write and use incorrectly, so it's much more efficient to write, debug, and maintain. — user4581301, Nov 22 '17 at 01:38

score 0 · Answer 2 · answered Nov 22 '17 at 02:49

After reading the OP's question and a decently provided answer worth a thumbs up for their help, but still is lacking in some answering only due to the OPs lack of submitting a Minimal, Complete, & Verifiable example I tend to think that the OP is a victim of:

As well as any other unseen issues or language specific grammatical errors that can not be seen by the community from source that wasn't submitted.

Access Violation Writing location 0x00000028

2 Answers2