toogreat4u 127 Report post Posted March 13, 2009 This is another post in reference to a polynomial using a queue style format and the problem I am having is how to program the overloaded operator = for the class. I think I am on the right track but it crashes everytime I try and run this program so I am kinda stuck. void Polynomial::operator =(const Polynomial& p) { if(p.head == NULL) head = tail = NULL; else { PolyPtr temp = p.head; head->coefficent = p.head->coefficent; head->power = p.head->power; head->link = NULL; tail = head; temp = temp->link; while(temp != NULL) { tail->link = temp; tail = temp; } } } Here is what head,tail, and PolyPtr refer to struct PolyNode { int power; int coefficent; PolyNode *link; }; typedef PolyNode *PolyPtr; class Polynomial { public: // Constructors Polynomial(); Polynomial(const Polynomial& p); Polynomial(int a); Polynomial(int a, int b); // Destructor ~Polynomial(); // overloaded = operator void operator =(const Polynomial& p); // addition friend Polynomial operator +(const Polynomial& p, const Polynomial& p2); // subtraction friend Polynomial operator -(const Polynomial& p, const Polynomial& p2); // multiplication friend Polynomial operator *(const Polynomial& p, const Polynomial& p2); // input friend istream& operator >>(istream& ins, Polynomial& p); // outputss friend ostream& operator <<(ostream& outs, const Polynomial& p); // this will react just like a insert in a queue void insert(int c, int p); // evaluation double eval(int a); private: PolyPtr head; PolyPtr tail; }; Any help would be truly appreciated. I had been posting about operator + but I found out that I had totally missed the operator = operations so therefore until I have that I can not adequately test my operator + function. Thanks for any help. 0 Share this post Link to post Share on other sites
toogreat4u 127 Report post Posted March 13, 2009 Found a couple things that muck up the code but still get the crashThis is the code that you should look at for the operator = function.void Polynomial::operator =(const Polynomial& p){ if(p.head == NULL) head = tail = NULL; else { PolyPtr temp = p.head; head->coefficent = temp->coefficent; head->power = temp->power; head->link = NULL; tail = head; temp = temp->link; while(temp != NULL) { tail->link = temp; tail = temp; temp = temp->link; } } } 0 Share this post Link to post Share on other sites
Blobberson 116 Report post Posted March 13, 2009 Can I ask why you're doing the list management yourself and not using a std::list<PolyNode> inside your polynomial class? 0 Share this post Link to post Share on other sites
Twisol 468 Report post Posted March 13, 2009 Firstly, I see no reason why your arithmetic operators couldn't be members instead of friends.Secondly, I can't tell without seeing the constructor(s), but you might want to check if it's possible for 'head' to point to invalid memory. You're assuming that 'head' is valid in body polynomials, and copying the head without checking for null. Here's how I'd do this code (assume we're in the 'else'):EDIT: I am an idiot. Your if/else checks for the head null thing. Well, at least this solves the deep/shallow copy problem and calls delete properly... the former of which could have caused the crash anyhow.// delete the old polynomialwhile (head != NULL){ PolyPtr temp = head->link; delete head; head = temp;}// Create at least one node to start// If 'p' happens to be empty, we'll still// have something valid in this one.tail = new PolyNode;tail->power = 1;tail->coefficient = 0;head = tail; // anchor the head to the new head // we're using tail to add onPolyPtr temp = p.head;while (temp != NULL){ // copy the node temp is pointing to // onto the node curr is pointing to *tail = *temp; // if there's another link still, if (temp->link != NULL) { // create a new polynode // we want to "deep-copy" the polynomial, so we have // separate memory locations for tail and temp tail->link = new PolyNode; // move forward tail = tail->link; }}I haven't tested it, but it looks solid. Could probably be done better, too, but at least it should work. (I hope it does. O_o) You also might want to look into step-by-step debugging to see exactly where it crashes.~JonathanEDIT: See edit above code.[Edited by - Twisol on March 13, 2009 3:57:04 PM] 0 Share this post Link to post Share on other sites
toogreat4u 127 Report post Posted March 13, 2009 Yeah, and although you did it differnetly Twisol you really helped me get back on the right track. Here is what I did but it's pretty similar to your suggestion.void Polynomial::operator =(const Polynomial& p){ if(p.head == NULL) head = tail = NULL; else { PolyPtr temp = p.head; head = new PolyNode; head->coefficent = temp->coefficent; head->power = temp->power; head->link = NULL; tail = head; temp = temp->link; while(temp != NULL) { PolyPtr temp2 = new PolyNode; temp2->coefficent = temp->coefficent; temp2->power = temp->power; temp2->link = NULL; tail->link = temp2; tail = temp2; temp = temp->link; } } } 0 Share this post Link to post Share on other sites
Twisol 468 Report post Posted March 13, 2009 Sure, that works. You're leaking memory though: every time you assign one polynomial to another, the previous contents of the polynomial on the left are lost and inaccessible, yet still allocated and containing information. That's why I made sure to run through the polynomial and delete everything first, before I copied the right polynomial over.Memory leaks are a bad idea, and if your memory space gets too cluttered it slows down your program, even if most of that allocated memory is actually unused.Glad I helped you out, though. [smile]~Jonathan 0 Share this post Link to post Share on other sites
NotAYakk 876 Report post Posted March 16, 2009 Try using the 'one true copy constructor': the copy-swap idiom.Writing a swap function for two polynomials is easy.Writing the copy constructor is easier than writing operator= (because the destructor already deals with destroying the elements).Your operator= then looks like:anyType& operator=(const anyType& other) { anyType tmp = other; this->swap(tmp); return *this;}The above code will be as exception safe as your swap (and swap shouldn't throw), copy constructor and destructor. It works for almost every class that uses pointer or value semantics on swap (reference semantics on swap breaks it).You have to write copy construct and destructors anyhow. And swap is easy to write. So you have just reduced code duplication.And in most cases, there is next to no overhead from the temporary object created by the copy constructor.And yes, you really should be thinking about using a std::list or the like. Writing a list by hand, as more than a learning exercise, is a bad idea....For the copy constructor, I'd write:PolyPtr clonePolyPtr( PolyPtr src ) { if (!src) return src; PolyPtr tmp = new PolyPtr(); tmp->coefficient = src->coefficient; tmp->power = src->power; tmp->link = clonePolyPtr( src->link ); return tmp;}(Note that making the above exception-safe is annoying. Which is another reason to use std::list.)Now your copy constructor is '0' lines long:Polynomial::Polynomial( const Polynomial& other ):head( clonePolyPtr(other.head) ) {}Swap is easy:void Polynomial::swap( Polynomial& other ) { std::swap( head, other.head );}And .. done. Well you need to write Polynomial::~Polynomial, but you would have had to do that anyhow.Going deeper down the rabbit hole, you could use boost::shared_ptr and make the cleanup code trivial (ie, a trivial destructor works). 0 Share this post Link to post Share on other sites