c++ Overwriting an already defined variable

Question

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 << ")";
    }       
 };

Read this: [The rule of 3](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) — Benjamin Lindley, Jan 27 '12 at 15:54
@Michael it will generate a temporary object which will be assigned to the first one. — AlexTheo, Jan 27 '12 at 15:57

JaredPar · Answer 1 · 2012-01-27T16:12:01.273

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

The constructor TermProd::TermProd(TermPtr, TermPtr) is executed
The values m_lhs and m_rhs are copied into the value x
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

Your assignment operator should check if you assign to yourself, and in that case typically do nothing. — Lindydancer, Jan 27 '12 at 16:08

score 3 · Answer 2 · edited May 23 '17 at 12:06

3

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.

edited May 23 '17 at 12:06

Community

1
1

answered Jan 27 '12 at 15:54

Alok Save

202,538
53
430
533

Lindydancer · Answer 3 · 2012-01-27T16:03:44.287

3

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;
}

edited Jan 27 '12 at 16:03

answered Jan 27 '12 at 15:55

Lindydancer

25,428
4
49
68

1

And then `clone()` can be simplified to `return new TermProd(*this);`. – Mike Seymour Jan 27 '12 at 15:58
I don't think that's the problem here. The faulty code is the assignment which will invoke `operator=` not the copy constructor – JaredPar Jan 27 '12 at 15:59
You'll also need a copy-assignment operator, `TermProd & operator=(TermProd const &);` – Mike Seymour Jan 27 '12 at 15:59
@JaredPar: Both *could* be a problem here - the first line does a copy, which may or may not be elided. – Mike Seymour Jan 27 '12 at 16:00
@MikeSeymour yes you're correct about that. I read right past the copy and saw it as a direct ctor call – JaredPar Jan 27 '12 at 16:01

c++ Overwriting an already defined variable

3 Answers3