CODEWITHSUNDEEP
c++pointersoverwrite
user1173818
user1173818

Reputation: 41

c++ Overwriting an already defined variable

I have the following main function, creating a product of coefficients using pointers. It's only a small part of the project, which is used to create polynomials:

#include "header.h"
int main()
{
    TermProd x = TermProd( new Coeff(4), new Coeff(8));
    x.print(); cout << endl;
    x = TermProd( new Coeff(8), new Coeff(15));
    x.print(); cout << endl;
    return 0;
}

After testing, the overwriting seems to be working. But when I call the print on x, I get a segmentation fault. I have been trying and staring at it for quite a while now, but I can't figure out the real problem. Also my searches didn't result into the right direction so I decided to create a small code-snippet which reproduces the error.

My header.h file looks like:

class Term{
public:
    Term(){};
    virtual ~Term(){};
        virtual Term* clone() const = 0;
    virtual void print() const = 0;
};

class Coeff:public Term{
    int m_val; //by def: >=0
public:
    // Constructor
    Coeff();
    Coeff(int val = 1):m_val(val)
    // Copy constructor
    Coeff* clone() const{return new Coeff(this->val());}
    // Destructor
    ~Coeff(){}
    // Accessors
    int val() const{return m_val;} ;
    // Print
    void print() const{cout << m_val; }
};

class TermProd:public Term{
    TermPtr m_lhs, m_rhs;
public:
    // Constructor
    TermProd(TermPtr lhs, TermPtr rhs):m_lhs(lhs), m_rhs(rhs){ }
    // Copy constructor
    TermProd* clone() const
    {
        return new TermProd(m_lhs->clone(), m_rhs->clone());
    }
    // Destructor
    ~TermProd(){ delete m_lhs;delete m_rhs;}
    // Accessors
    Term* lhs() const { return m_lhs; }
    Term* rhs() const { return m_rhs; } 
    // Print
    void print() const
    {
        cout << "("; m_lhs->print(); cout << '*'; m_rhs->print(); cout << ")";
    }       
 };

Upvotes: 4

Views: 10372

Answers (3)

JaredPar
JaredPar

Reputation: 754715

Note here you are not overwriting the x variable but instead assigning to it. This will invoke the default operator= for your type which roughly causes the following code to be executed

  1. The constructor TermProd::TermProd(TermPtr, TermPtr) is executed
  2. The values m_lhs and m_rhs are copied into the value x
  3. The destructor for the value created in step #1 is now run and m_lhs and m_rhs are deleted

At this point you have a real problem. After step #2 both the value x and the temporary value created in step #1 shared the same m_lhs and m_rhs values. The destructor at step #3 deletes them yet x still has a reference to them which is now effectively pointing at dead memory

In order to fix this problem you will need to add your own operator= which correctly handles the assignment semantics. For example

TermProd& operator=(const TermProd& other) {
  if (&other != this) {
    delete m_lhs;
    delete m_rhs;

    m_lhs = other.m_lhs->clone();
    m_rhs = other.m_rhs->clone();
  }
  return *this;
};

In order to be correct for all scenarios you'll also need to add a proper copy constructor

TermProd::TermProd(const TermProd& other) :
  m_lhs(other.m_lhs->clone()),
  m_rhs(other.m_rhs->clone())
{

}

Really though to make this extremely simple you should consider using std::shared_ptr<Term> as the value for TermPtr. It's a pointer that will make the sharing work without all of the above mentioned overhead

Upvotes: 6

Lindydancer
Lindydancer

Reputation: 26094

You don't provide a copy constructor. You have a clone method, that the comment say is the copy constructor, but it isn't.

Try something like:

TermProd(TermProd const & other)
  m_lhs(other.m_lhs->clone()),
  m_rhs(other.m_rhs->clone())
{
}

And likewise for the other classes.

Update As pointed out in the comments, you will also need an assignment operator.

TermProd & operator=(TermProd const & other)
{
  if (this != &other)  // Check for assignment to self.
  {
    delete m_lhs;
    delete m_rhs;
    m_lhs = other.m_lhs->clone();
    m_rhs = other.m_rhs->clone();
  }
  return *this;
}

Upvotes: 3

Alok Save
Alok Save

Reputation: 206528

You are not following the Rule of Three.
The destruction of temporary variables created during the function calls by calling the implicit copy constructor deletes the allocated pointer member.

You need to provide your own copy constructor and copy assignment operator which will make a deep copies of the dynamically allocated pointer member.

Upvotes: 3

Related Questions