0

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.

3
  • 1
    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) Aug 12, 2011 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, 2011 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"? Aug 12, 2011 at 21:37

5 Answers 5

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.

4
  • "Must" is perhaps too strong. "Should" is more appropriate. Aug 12, 2011 at 19:44
  • @Oli Charlesworth: I agree, though in this case the must is a must :)
    – Alok Save
    Aug 12, 2011 at 19:47
  • @Als: Very true! Anyway, +1. Aug 12, 2011 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. Aug 12, 2011 at 20:10
2

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.

1
  • 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, 2011 at 22:04
0

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

0
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.

0
0

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)

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.