473,320 Members | 2,097 Online
Bytes | Software Development & Data Engineering Community
Post Job

Home Posts Topics Members FAQ

Join Bytes to post your question to a community of 473,320 software developers and data experts.

destructor being called for the * operator

31
I overloaded the * operator function as follows:

Expand|Select|Wrap|Line Numbers
  1. Polynomial Polynomial::operator*(const Polynomial &p) {
  2.     Polynomial newPoly;
  3.     Term * temp1 = this->head;
  4.     Term * temp2 = p.head;
  5.     double c = 0;
  6.     int d = 0;
  7.  
  8.     for (int i = 0; i < this->numTerms; i++) {
  9.         for(int j = 0; j < p.numTerms; j++) {
  10.             c = temp1->coeff * temp2->coeff;
  11.             d = temp1->deg + temp2->deg;
  12.  
  13.             newPoly.prepend(c, d);
  14.             temp2 = temp2->next;
  15.             newPoly.numTerms += 1;
  16.         }
  17.         temp1 = temp1->next;
  18.         temp2 = p.head;
  19.     }
  20.  
  21.     return newPoly;
  22. }
  23.  
now there is nothing wrong with the code, through the debugger, I saw that the returned polynomial is correct.

However, when I call the operator in main

such as
Expand|Select|Wrap|Line Numbers
  1.     Polynomial p1;
  2.     inFile >> p1;
  3.  
  4.     Polynomial p2;
  5.     inFile >> p2;
  6.  
  7.     Polynomial p3 = p1*p2;
  8.     cout << "p3 = " << p3 << endl;
  9.  
  10.  
for some reason, the destructor is called, and I get garbage value for p3 instead of p1*p2.

I'm thinking it may have something to do with either the copy constructor or the overloaded= operator I created.

Here they are:
Expand|Select|Wrap|Line Numbers
  1. //copy constructor
  2. Polynomial::Polynomial (const Polynomial & p) {
  3.     this->numTerms = p.numTerms;
  4.     this->head = p.head;
  5. }
  6.  
Expand|Select|Wrap|Line Numbers
  1. //Polynomial assignment operator
  2. const Polynomial & Polynomial::operator=(const Polynomial & rhs) {
  3.     assert (this->numTerms == rhs.numTerms);
  4.  
  5.     if (&rhs != this) {
  6.         this->head = rhs.head;
  7.     }
  8.  
  9.     return *this;
  10. }
  11.  
can anyone give a reason why this is happening?
Oct 13 '07 #1
11 1672
blackx
31
if I removed the destructor from my code, I get the answer that I wanted.

Expand|Select|Wrap|Line Numbers
  1. //destructor
  2. Polynomial::~Polynomial () {
  3.     while (this->head != 0) {
  4.         del();
  5.     }
  6.  
  7.     this->numTerms = 0;
  8. }
  9.  
Oct 13 '07 #2
blackx
31
how should I solve this if I want to have a destructor in my code? I'm thinking the destructor is called because the scope of the function ended.

Should I be using something like static or const?

help please.
Oct 13 '07 #3
blackx
31
I solved the problem.

I created a pointer for newPoly instead.

Expand|Select|Wrap|Line Numbers
  1. Polynomial Polynomial::operator*(const Polynomial &p) {
  2.     //the newPoly polynomial will be the product of our two polynomials
  3.     Polynomial * newPoly = new Polynomial;  
  4.  
  5.     Term * temp1 = this->head;
  6.     Term * temp2 = p.head;
  7.  
  8.     double c = 0;
  9.     int d = 0;
  10.  
  11.     //we first multiply the two polynomials term by term
  12.     for (int i = 0; i < this->numTerms; i++) {
  13.         for(int j = 0; j < p.numTerms; j++) {
  14.             c = temp1->coeff * temp2->coeff;
  15.             d = temp1->deg + temp2->deg;
  16.  
  17.             newPoly->prepend(c, d);
  18.             temp2 = temp2->next;
  19.             newPoly->numTerms += 1;
  20.         }
  21.         temp1 = temp1->next;
  22.         temp2 = p.head;
  23.     }
  24.  
  25.     //we now combine Terms with the same degrees
  26.     newPoly->combineTerms();
  27.  
  28.     return *newPoly;
  29. }
  30.  
I would just like to understand why this is so? the pointer is not deleted by the destructor?
Oct 13 '07 #4
Banfa
9,065 Expert Mod 8TB
I solved the problem.

I created a pointer for newPoly instead.

I would just like to understand why this is so? the pointer is not deleted by the destructor?
OK blackx, firstly this is a multinational you may well not get an answer for up to 24 hours as the expet answering your question may not be in the same time zone as you.


As to your question and solution, your solution is not a solution. It is adding another bug on top of already buggy code.


Starting with you original operator* and original question, the reason that the delete operator is called when the function returns is that the function returns Polynomial, this means that a temporary object is created on the heap to hold this return value and as soon as it goes out of scope (at the end of the calling statement) the compiler deletes it.

The reason that you get garbage in p3 is a little more involved and I am going to have to make some guesses. I guess that Polynomial::head is a pointer and that you allocate data for it (call new) in the constructor (and possibly in other places) and you delete it in the destructor.

Now your copy constructor (and assignment operator??) just copies the value of head from one object to another, so both objects end up pointing at the same block of allocated memory. This is the root your problem, if you create a Polynomial, newPoly in operator*, and then construct another Polynomial from it , as happens when operator* returns to create the temporary variable then both of the 2 instances of Polynomial have the head pointing at the same piece of allocated data. If one of those 2 instances then gets deleted, as happens to newPoly when operator* returns, then the that memory that head points to is deleted and the other instance of Polynomial ends up with an invalid head pointer.

What your "solution" does is rather than create newPoly on the stack and have the compiler automatically delete it you have created it on the heap using new. When you do this it is your responsibility to delete it (or put in place a mechanism that does it for you like smart pointers). However you never delete it, because it doesn't get deleted you have a memory leak. You get the answer you want because there is no temporary object created (or the object is a pointer rather than a Polynomial) so no object referring to that particular head is deleted, newPoly is not deleted and a temporary Polynomial is not created and deleted. Therefore the memory that head points to in p3 is not deleted at any time so it is valid and p3 apparently has the correct answer.

But you do have a memory leak which will ultimately bring your program crashing down.


The correct solution to your problem is in the copy constructor and assignment operator (if you need one your normally need the other), when copying a Polynomial from 1 object to another rather than copying head (a flat copy) what you should be doing is allocating new data for the destination polynomial and copying the data that head points to. Then when an Polynomial object is deleted and head is deleted it will not effect any other objects as they each have their own memory.
Oct 13 '07 #5
blackx
31
thanks for the reply. I understand what you meant by the memory leak. I have to clean up my code now.

Thanks again.
Oct 13 '07 #6
blackx
31
what I did is this, changing the copy constructor and the = operator:

Expand|Select|Wrap|Line Numbers
  1. //copy constructor
  2. Polynomial::Polynomial (const Polynomial & p) {
  3.     int newNumTerms;
  4.     Term * newHead;
  5.  
  6.     newNumTerms = p.numTerms;
  7.     this->numTerms = newNumTerms;
  8.  
  9.     newHead = p.head;
  10.     this->head = newHead;
  11. }
  12.  
Expand|Select|Wrap|Line Numbers
  1. //Polynomial assignment operator
  2. const Polynomial & Polynomial::operator=(const Polynomial & rhs) {
  3.     assert (this->numTerms == rhs.numTerms);
  4.     Term * newHead;
  5.  
  6.     if (&rhs != this) {
  7.         newHead = rhs.head;
  8.         this->head = newHead;
  9.     }
  10.  
  11.     return *this;
  12. }
  13.  
However, I got garbage value for the coefficients of the polynomial. The degrees were correct though., as well as the coefficient for the last term.

I used the earlier code I gave for the * operator.
Oct 13 '07 #7
weaknessforcats
9,208 Expert Mod 8TB
//copy constructor
Polynomial::Polynomial (const Polynomial & p) {
int newNumTerms;
Term * newHead;

newNumTerms = p.numTerms;
this->numTerms = newNumTerms;

newHead = p.head;
this->head = newHead;
}
Re-read what Banfa said. Here you are still not making a copy. Instead, you create a local variable, copy the address in p.head to it and then assign the variable to this->head. Result: You just copied the pointer. You might as well have coded:
Expand|Select|Wrap|Line Numbers
  1. //copy constructor
  2. Polynomial::Polynomial (const Polynomial & p) {
  3.    this->numTerms = p.numTerms;
  4.    this->head = p.head;
  5. }
  6.  
Since the compiler knows how to copy pointers, you wouldn't need the copy constructor at all.

You simply cannot copy pointers in C++.

You need to make a copy of the Polynomial:
1) create a new linked list for the this object and copy the data from the p object to the this object.
2) when you are done you should have two objects, each with its own linked list and each with its own copy of the data.

Your assignment operator has the same problem. It doesn't work either.
Oct 13 '07 #8
blackx
31
do you mean something like this?

Expand|Select|Wrap|Line Numbers
  1. //copy constructor
  2. Polynomial::Polynomial (const Polynomial & p) {
  3.     Polynomial newP;
  4.  
  5.     newP.numTerms = p.numTerms;
  6.     this->numTerms = newP.numTerms;
  7.  
  8.     newP.head = p.head;
  9.     this->head = newP.head;
  10. }
Using this resulted to runtime errors again :(
Oct 13 '07 #9
Ganon11
3,652 Expert 2GB
No, you're still doing the same thing.

You cannot set this->head to p->head - that's just performing the same problem weaknessforcats explained.

You need to go through each node in the linked list, manually create a new node for the this polynomial, and manually copy the values stored in each node, not the pointers.
Oct 13 '07 #10
Banfa
9,065 Expert Mod 8TB
I think an example is in order and at the risk of having WeaknessForCats pick apart my code here one is

Expand|Select|Wrap|Line Numbers
  1. #include <iostream>
  2.  
  3. using namespace std;
  4.  
  5. class BadCopy
  6. {
  7. private:
  8.     int *Data;
  9.  
  10. public:
  11.     BadCopy(int init=0)
  12.     {
  13.         this->Data = new int;
  14.         *this->Data = init;
  15.     }
  16.  
  17.     BadCopy(const BadCopy &copy)
  18.     {
  19.         this->Data = copy.Data;
  20.     }
  21.  
  22.     const BadCopy &operator=(const BadCopy &assign)
  23.     {
  24.         this->Data = assign.Data;
  25.  
  26.         return *this;
  27.     }
  28.  
  29.     int GetValue()
  30.     {
  31.         return *this->Data;
  32.     }
  33.  
  34.     ~BadCopy()
  35.     {
  36.         delete this->Data;
  37.     }
  38. };
  39.  
  40. class GoodCopy
  41. {
  42. private:
  43.     int *Data;
  44.  
  45. public:
  46.     GoodCopy(int init=0)
  47.     {
  48.         this->Data = new int;
  49.         *this->Data = init;
  50.     }
  51.  
  52.     GoodCopy(const GoodCopy &copy)
  53.     {
  54.         this->Data = new int;
  55.         *this->Data = *copy.Data;
  56.     }
  57.  
  58.     const GoodCopy &operator=(const GoodCopy &assign)
  59.     {
  60.         delete this->Data;
  61.  
  62.         this->Data = new int;
  63.         *this->Data = *assign.Data;
  64.  
  65.         return *this;
  66.     }
  67.  
  68.     int GetValue()
  69.     {
  70.         return *this->Data;
  71.     }
  72.  
  73.     ~GoodCopy()
  74.     {
  75.         delete this->Data;
  76.     }
  77. };
  78.  
  79. int main()
  80. {
  81.     BadCopy bc1(5), bc2 = 10, *pbc;
  82.     GoodCopy gc1(5), gc2 = 10, *pgc;
  83.  
  84.     pbc = new BadCopy(20);
  85.     pgc = new GoodCopy(20);
  86.  
  87.     cout << "BadCopy : " << bc1.GetValue() << ", " << bc2.GetValue() << ", " << pbc->GetValue() << endl;
  88.     cout << "GoodCopy: " << gc1.GetValue() << ", " << gc2.GetValue() << ", " << pgc->GetValue() << endl;
  89.  
  90.     bc1 = bc2;
  91.     bc2 = *pbc;
  92.     delete pbc;
  93.  
  94.     gc1 = gc2;
  95.     gc2 = *pgc;
  96.     delete pgc;
  97.  
  98.     cout << "BadCopy : " << bc1.GetValue() << ", " << bc2.GetValue() << endl;
  99.     cout << "GoodCopy: " << gc1.GetValue() << ", " << gc2.GetValue() << endl;
  100.  
  101.     return 0;       
  102. }
  103.  
Firstly note this code assumes that the new operator doesn't fail. In a production application you should not make this assumption but should trap and handle appropriately the exception that new throws if it can not allocate the memory.


In this example I define 2 classes, BadCopy and GoodCopy. BadCopy basically does what you are currently doing, it copies points from object to object. GoodCopy is what we are saying you should do, rather than copying pointers it allocates new data and copies the data pointed at rather than the pointer.

Since accessing an invalid pointer (which is what happens on the 2nd bc cout line) is undefined behaviour how this application runs rather depends on the platform.

However on Visual Studio 2005 Express in a debug build what you get is corrupt data output for bc2 on the second time and a memory trap when main exits and bc2 is destructed as it attempts to delete already deleted memory.
Oct 13 '07 #11
weaknessforcats
9,208 Expert Mod 8TB
I think an example is in order and at the risk of having WeaknessForCats pick apart my code
Ah, that's my cue.

The GoodCopy assignment operator should check that the object is not being assigned to itself. If it is, then the delete of the data will delete the very data that's going to be assigned. Assignmment operators should not allow self-assignment:
Expand|Select|Wrap|Line Numbers
  1. const GoodCopy &operator=(const GoodCopy &assign)
  2.     {
  3.         if (this == &assign) return *this;  //no self-assignment
  4.         delete this->Data;
  5.  
  6.         this->Data = new int;
  7.         *this->Data = *assign.Data;
  8.  
  9.         return *this;
  10.     }
  11.  
Oct 14 '07 #12

Sign in to post your reply or Sign up for a free account.

Similar topics

52
by: Newsnet Customer | last post by:
Hi, Statement 1: "A dynamically created local object will call it's destructor method when it goes out of scope when a procedure returms" Agree. Statement 2: "A dynamically created object...
9
by: sahukar praveen | last post by:
Hello, This is the program that I am trying. The program executes but does not give me a desired output. ********************************************** #include <iostream.h> #include...
4
by: Joe | last post by:
I am looking for the quintessential blueprint for how a C++ like destructor should be implemented in C#. I see all kinds of articles in print and on the web, but I see lots of discrepencies. For...
6
by: MirzaD | last post by:
Hello Below you will find the problematic program. It is a string wrapper class with a bare minimum of functionality to keep things simple. **Code** //StringClass.h #include <iostream>...
4
by: Michael | last post by:
Hello, I want to use an object (LowCut) within another object (SampleRateConverter) like it is written as follows: class SampleRateConverter { public: SampleRateConverter( int...
5
by: junw2000 | last post by:
I use the code below to study delete and destructor. #include <iostream> using namespace std; struct A { virtual ~A() { cout << "~A()" << endl; }; //LINE1 void operator delete(void* p) {...
9
by: rohits123 | last post by:
I have an overload delete operator as below ////////////////////////////////// void operator delete(void* mem,int head_type) { mmHead local_Head = CPRMemory::GetMemoryHead(head_type);...
7
by: michael | last post by:
Hi All, I have written the following to illustrate a problem. I know I have some magic numbers etc please ignore them. What I do not follow is why the line marked results in a call to the...
12
by: Rahul | last post by:
Hi Everyone, I have the following code and i'm able to invoke the destructor explicitly but not the constructor. and i get a compile time error when i invoke the constructor, why is this so? ...
9
by: itdevries | last post by:
Hi, I've ran into some trouble with an overloaded + operator, maybe someone can give me some hints what to look out for. I've got my own custom vector class, as a part of that I've overloaded...
0
by: ryjfgjl | last post by:
ExcelToDatabase: batch import excel into database automatically...
0
isladogs
by: isladogs | last post by:
The next Access Europe meeting will be on Wednesday 6 Mar 2024 starting at 18:00 UK time (6PM UTC) and finishing at about 19:15 (7.15PM). In this month's session, we are pleased to welcome back...
1
isladogs
by: isladogs | last post by:
The next Access Europe meeting will be on Wednesday 6 Mar 2024 starting at 18:00 UK time (6PM UTC) and finishing at about 19:15 (7.15PM). In this month's session, we are pleased to welcome back...
0
by: jfyes | last post by:
As a hardware engineer, after seeing that CEIWEI recently released a new tool for Modbus RTU Over TCP/UDP filtering and monitoring, I actively went to its official website to take a look. It turned...
1
by: PapaRatzi | last post by:
Hello, I am teaching myself MS Access forms design and Visual Basic. I've created a table to capture a list of Top 30 singles and forms to capture new entries. The final step is a form (unbound)...
1
by: Shællîpôpï 09 | last post by:
If u are using a keypad phone, how do u turn on JavaScript, to access features like WhatsApp, Facebook, Instagram....
0
by: af34tf | last post by:
Hi Guys, I have a domain whose name is BytesLimited.com, and I want to sell it. Does anyone know about platforms that allow me to list my domain in auction for free. Thank you
0
by: Faith0G | last post by:
I am starting a new it consulting business and it's been a while since I setup a new website. Is wordpress still the best web based software for hosting a 5 page website? The webpages will be...
0
isladogs
by: isladogs | last post by:
The next Access Europe User Group meeting will be on Wednesday 3 Apr 2024 starting at 18:00 UK time (6PM UTC+1) and finishing by 19:30 (7.30PM). In this session, we are pleased to welcome former...

By using Bytes.com and it's services, you agree to our Privacy Policy and Terms of Use.

To disable or enable advertisements and analytics tracking please visit the manage ads & tracking page.