C++ Copy Constructor Strange Behavior

Question

Given the following code:

class monomial {
public:
    mp_real coe;
    int exp;        
    monomial *next; 
};

class polynomial  
{  
private:
    monomial *start;
public:
    polynomial();
    ~polynomial();
    void destroy_poly(monomial *);
    polynomial & operator=(const polynomial &);
    polynomial(const polynomial&);
    monomial * get_start();
};  
polynomial::polynomial() {
    start = NULL;
}
polynomial::~polynomial() {
    if (start != NULL) {
        destroy_poly(start);
    }
    start = NULL;
}
void
polynomial::destroy_poly(monomial * m) {
    monomial * cm = m;
    if (cm->next != NULL) {
        destroy_poly(cm->next);
    }
    delete m;
}
polynomial::polynomial(const polynomial& p) {
    if (p.start != NULL) {
        start = new monomial();
        start->coe = p.start->coe;
        start->exp = p.start->exp;

        monomial * tmpPtr = p.start;
        monomial * lastPtr = start;

        while (tmpPtr->next != NULL) {
            monomial * newMon = new monomial();
            newMon->coe = tmpPtr->next->coe;
            newMon->exp = tmpPtr->next->exp;

            lastPtr->next = newMon;
            lastPtr = lastPtr->next;
            tmpPtr = tmpPtr->next;
        }
    }
}
polynomial & polynomial::operator=(const polynomial &p) {
    if (p.start != NULL) {
        start = new monomial();
        start->coe = p.start->coe;
        start->exp = p.start->exp;

        monomial * tmpPtr = p.start;
        monomial * lastPtr = start;

        while (tmpPtr->next != NULL) {
            monomial * newMon = new monomial();
            newMon->coe = tmpPtr->next->coe;
            newMon->exp = tmpPtr->next->exp;

            lastPtr->next = newMon;
            lastPtr = lastPtr->next;
            tmpPtr = tmpPtr->next;
        }
    }
    return *this;
}

Then in main.cpp:

main() {
    polynomial poly;
    // code that initializes / sets values for 
    // various monomials pointed to by poly.start.

    map<set<unsigned int>, polynomial> hash;
    set<unsigned int> tmpSet;

    tmpSet.insert(0);

    hash[tmpSet] = poly;
}

I can set a breakpoint on the first line of the copy constructor, and after I step-over the hash[tmpSet] = poly line, p.start inside the copy constructor is NULL. Then, it gets called a second time, at which time p.start has strange values set inside it.

Any clue what is going on?

Thanks, Erich

EDIT 1: Thought adding the assignment operator to the polynomial class fixed it, but it did not. Still having the same problem.

You fixed the assignment operator, but still not if p.start is null, which is the default. Anywhere with `if (p.start != NULL) {` should end with `} else { start = NULL;}` (well, don't forget to deallocate in the assignment operator first) — Mooing Duck, Aug 12 '11 at 21:31
Well maybe I am wrong or something. Please don't call me stupid, but look. You put in hash container object which has inside pointers. Putting object in container makes it copy (in your case only pointer address). when program goes out of scope it tries to destroy pointer few times. So there you should use some pointer with reference count or something. Correct if I am wrong. — legion, Aug 12 '11 at 21:36
Thanks. I added the else statement and it is working now. Could you explain a little bit more why this fixed the problem? Also, what do you mean by "deallocate in the assignment operator first"? — Erich Peterson, Aug 12 '11 at 21:37

score 4 · Accepted Answer · edited May 23 '17 at 12:11

4

You are violating the Rule of Three:
Since you overload Copy constructor, You Should overload copy assignment operator and destructor for your class.

The above statement was in reference to general scenarios, In your code example, you can replace the should with a must.

edited May 23 '17 at 12:11

Community

1
1

answered Aug 12 '11 at 19:37

Alok Save

202,538
53
430
533

"Must" is perhaps too strong. "Should" is more appropriate. – Oliver Charlesworth Aug 12 '11 at 19:44
@Oli Charlesworth: I agree, though in this case the **must** is a must :) – Alok Save Aug 12 '11 at 19:47
@Als: Very true! Anyway, +1. – Oliver Charlesworth Aug 12 '11 at 19:49
Sorry. I thought that adding the assignment operator to the polynomial class fixed the problem but it didn't. I've updated the code with the new additions. – Erich Peterson Aug 12 '11 at 20:10

score 2 · Answer 2 · answered Aug 12 '11 at 19:39

Two issues.

1) You don't have a no-argument constructor, so the compiler is filing in the initial values as it sees fit. The fact that p.start is initially NULL is

2) When your copy constructor passed a polynomial with p.start == NULL, you don't initialize any of the variables in the class, so the next copy could have whatever initial value the compiler assigns (see problem #1).

To fix, you should add a no-arg constructor that initializes the polynomial into a sane state. And then the copy constructor, if passed such a polynomial, should initialize itself to a sane state.

The second point still stands. If you pass an empty polynomial to the copy constructor, it sees that `p.start` is NULL. It then never sets `this->start` to NULL (the default constructor is not run during copy constructor). — Dave S, Aug 12 '11 at 22:04

score 0 · Answer 3 · answered Aug 12 '11 at 19:39

0

You should add a default constructor, where you initialize start to NULL.

answered Aug 12 '11 at 19:39

korbes

1,273
1
12
18

score 0 · Answer 4 · answered Aug 12 '11 at 19:39

0

As Als pointed out, you violating the Rule of Three. Well look, when you put something in container what happens to your pointers .

monomial *start;
monomial *next;

Think about this.

answered Aug 12 '11 at 19:39

legion

199
1
4
13

score 0 · Answer 5 · answered Aug 12 '11 at 19:53

Your copy constructor leaves start set to some random value if p.start == NULL. That might be the cause of the problem, depending on other code in your program. Also, you have no assignment operator, so the compiler is doing shallow copies for you. You have to add another function polynomial &operator=(const polynomial& b)

C++ Copy Constructor Strange Behavior

5 Answers5