CODEWITHSUNDEEP
c++
w0nderwall
w0nderwall

Reputation: 25

C++ overloading the '=' operator cause crash

I have a problem on my simple function to overload the operator '='. Here's the code: class definition:

class SeqMat{
private:
      elem* punt;
      SeqMat(const SeqMat&);
public:
      SeqMat();
      void insMat(int, int);
      friend ostream& operator<<(ostream& os, const SeqMat&);
      SeqMat& operator-=(int);
      ~SeqMat();
      SeqMat& operator~();
      SeqMat& operator=(const SeqMat&);
};

Function:

SeqMat& SeqMat::operator=(const SeqMat& lista){
    if (this != &lista){
        delete punt;
        punt = lista.punt;
    }
    return *this;
}

When I run the compiled .exe, the program stops working like in this screen: enter image description here

Here's the right code:

SeqMat& SeqMat::operator=(const SeqMat& lista) {
elem* punt1;
elem* punt2;
punt2 = lista.punt;
punt1 = new elem;
delete punt;
punt = punt1;
while (punt2 != nullptr){
    punt1->numero = punt2->numero;
    punt1->ripetizioni = punt2->ripetizioni;
    punt2 = punt2->pun;
    if (punt2 == nullptr){
        punt1->pun = nullptr;
        break;
    }
    else{
        punt1->pun = new elem;
        punt1 = punt1->pun;
    }
}
punt1 = nullptr;
return *this;

}

Upvotes: 1

Views: 1240

Answers (2)

vmcloud
vmcloud

Reputation: 652

As you do not show your main executed code, I do not know if the crash has directly relation with the operator= overloading. But it still has some problem with your code:

  1. Generally, you should use smart pointer if you wrap some resource in you class. Here, if punt is created by planned or in other place, we can see it as resource.
  2. You should definitely know what do you want in the operator= overloading(shallow copy or deep copy).

(1) if punt is a type of resource, shallow copy required, and shallow copy is always delegated by smart pointer here(shared_ptr, unique_ptr or other).

(2) if punt is not a type of resource, and you want to use deep copy, a typical deep copy code should like this:

SeqMat& SeqMat::operator=(const SeqMat& lista) {
    if (this != &lista){
        SeqMat *pOrgin = punt;        //remember the origin pointer, to confirm that the punt get the correct value then deleted
        punt = new elem(lista.punt); // Make a COPY
        delete pOrigin;
    }
    return *this;
}

Upvotes: 0

Drew Dormann
Drew Dormann

Reputation: 63912

This function

SeqMat& SeqMat::operator=(const SeqMat& lista)

is a "copy" operation. It is making *this be a copy of lista.

As such, you shouldn't be delete-ing more than you are new-ing. Copying shouldn't make fewer things exist.

As such - based on the code you're showing - it should look something like:

SeqMat& SeqMat::operator=(const SeqMat& lista){
    if (this != &lista){
        delete punt;
        punt = new elem(lista.punt); // Make a COPY
    }
    return *this;
}

At the very least, you should be thinking about whether elem* punt should be unique for each object or shared.

If it's unique for each object, you shouldn't share pointers to the same data. And if it's shared, you shouldn't be so eager to have one object delete the pointer.

Upvotes: 3

Related Questions