 |
BorlandTalk.com
Borland discussion newsgroups
|
| View previous topic :: View next topic |
| Author |
Message |
Spine
Guest
|
 Posted: Wed May 16, 2007 12:20 am Post subject: weird constructor - destructor problem |
 |
|
Hi
I have an interesting problem. I have defined a class with the following
constructor and destructor:
TNumber::TNumber()
{
Size = 10;
Value = new int[Size];
for (int l=0;l<Size;l++) Value[l] = 0;
}
TNumber::~TNumber()
{
delete [] Value;
}
now when I create an instance of this class, I have a array of Value. and
when then instance runs out of scope the destructor is called and the memory
is freed. everything nice, everything fine. It works fine for a while (maybe
up to 10.000 times created/destructed), then the program hangs itself. by
following the program I found out that the "Value = new int[Size];" line
actually calls the destructor via a lot of borland code. Then it ends up in
the following borland code loop:
@LockBlockTypeLoop:
mov eax, $100
{Attempt to grab the block type}
lock cmpxchg TSmallBlockType([ebx]).BlockTypeLocked, ah
je @GotLockOnSmallBlockType
{Couldn't grab the block type - sleep and try again}
push ecx
push edx
push InitialSleepTime
call Sleep
pop edx
pop ecx
{Try again}
mov eax, $100
{Attempt to grab the block type}
lock cmpxchg TSmallBlockType([ebx]).BlockTypeLocked, ah
je @GotLockOnSmallBlockType
{Couldn't grab the block type - sleep and try again}
push ecx
push edx
push AdditionalSleepTime
call Sleep
pop edx
pop ecx
{Try again}
jmp @LockBlockTypeLoop
which will just keep on running. Is this a bug, or did I do something stupid
(I actually found a very similar example in the help file..)? Can someone
help me with this?
Spike |
|
| Back to top |
|
 |
Chris Uzdavinis (TeamB)
Guest
|
| Posted: Wed May 16, 2007 12:54 am Post subject: Re: weird constructor - destructor problem |
|
|
"Spine" <boterkuipje (AT) hotmail (DOT) com> writes:
| Quote: | Hi
I have an interesting problem. I have defined a class with the following
constructor and destructor:
TNumber::TNumber()
{
Size = 10;
Value = new int[Size];
for (int l=0;l<Size;l++) Value[l] = 0;
}
TNumber::~TNumber()
{
delete [] Value;
}
now when I create an instance of this class, I have a array of Value. and
when then instance runs out of scope the destructor is called and the memory
is freed. everything nice, everything fine. It works fine for a while (maybe
up to 10.000 times created/destructed), then the program hangs itself. by
|
You don't provide nearly enough information to get any reasonable
feedback. We don't see your class declaration, nor do we see how
you're using it.
If I had to guess, you're making copies of TNumber and have a pointer
aliasing bug. Did you perhaps forget to implement the copy assignment
operator and the copy constructor?
[Unrelated side note: I would *highly* suggest you not use 'l' as a
variable name. It looks far too much like a '1', and such terseness
buys you nothing.]
--
Chris (TeamB); |
|
| Back to top |
|
|
Spine
Guest
|
| Posted: Wed May 16, 2007 2:04 am Post subject: Re: weird constructor - destructor problem |
|
|
"Chris Uzdavinis (TeamB)" <chris (AT) uzdavinis (DOT) com> wrote in message
news:86646tsw0w.fsf (AT) explicit (DOT) atdesk.com...
| Quote: | "Spine" <boterkuipje (AT) hotmail (DOT) com> writes:
Hi
I have an interesting problem. I have defined a class with the following
constructor and destructor:
TNumber::TNumber()
{
Size = 10;
Value = new int[Size];
for (int l=0;l<Size;l++) Value[l] = 0;
}
TNumber::~TNumber()
{
delete [] Value;
}
now when I create an instance of this class, I have a array of Value. and
when then instance runs out of scope the destructor is called and the
memory
is freed. everything nice, everything fine. It works fine for a while
(maybe
up to 10.000 times created/destructed), then the program hangs itself. by
You don't provide nearly enough information to get any reasonable
feedback. We don't see your class declaration, nor do we see how
you're using it.
If I had to guess, you're making copies of TNumber and have a pointer
aliasing bug. Did you perhaps forget to implement the copy assignment
operator and the copy constructor?
[Unrelated side note: I would *highly* suggest you not use 'l' as a
variable name. It looks far too much like a '1', and such terseness
buys you nothing.]
--
Chris (TeamB);
|
yes, I was affraid I did not provide enough information.
this is the declaration:
class TNumber
{
private:
int Size;
int *Value;
int exp;
int sign;
void Clean();
public:
TNumber();
~TNumber();
void Destroy();
void operator=(TNumber);
void operator=(double);
TNumber operator+(TNumber);
TNumber operator+(double);
TNumber operator-(TNumber);
TNumber operator-(double);
TNumber operator*(TNumber);
TNumber operator*(double);
TNumber operator/(TNumber);
TNumber operator/(double);
bool operator==(TNumber);
bool operator==(double);
bool operator<(TNumber);
AnsiString ToStr();
double ToDouble();
};
this is one of the overridden operators:
TNumber TNumber::operator+(double other)
{
TNumber result;
(code here)
return result;
}
I cannot actually free the momory associated with the result by calling a
destroy function, as return directly exits the function...
| Quote: | If I had to guess, you're making copies of TNumber and have a pointer
aliasing bug. Did you perhaps forget to implement the copy assignment
operator and the copy constructor?
probably not. how do I do this, and what exactly is the effect? |
Spike |
|
| Back to top |
|
|
Chris Uzdavinis (TeamB)
Guest
|
| Posted: Wed May 16, 2007 3:08 am Post subject: Re: weird constructor - destructor problem |
|
|
"Spine" <boterkuipje (AT) hotmail (DOT) com> writes:
| Quote: | yes, I was affraid I did not provide enough information.
this is the declaration:
class TNumber
{
private:
int Size;
int *Value;
int exp;
int sign;
void Clean();
public:
TNumber();
~TNumber();
void Destroy();
void operator=(TNumber);
void operator=(double);
TNumber operator+(TNumber);
TNumber operator+(double);
TNumber operator-(TNumber);
TNumber operator-(double);
TNumber operator*(TNumber);
TNumber operator*(double);
TNumber operator/(TNumber);
TNumber operator/(double);
bool operator==(TNumber);
bool operator==(double);
bool operator<(TNumber);
AnsiString ToStr();
double ToDouble();
};
|
Ok, now for some "real" feedback. :)
Things you should change:
1) your operator=(TNumber) is taking TNumber by value, which is thus
constructed by the copy constructor, which is generated by the
compiler.
This should be taking a reference to a const TNumber:
void operator=(TNumber const &);
2) Your operator==(TNumber) is needlessly taking a copy of the
argument, rather than a reference to the original. This also
exacerbates the problem.
3) Your class lacks a copy constructor:
TNumber(TNumber const & other);
The problem is, when you make a copy by value, and you use the
compiler generated copy constructor, it copies all the members,
including the pointers. Thus, two objects will hold the same address
in its Value pointer. The first one that goes out of scope will
delete the memory. The second one, which aliases the other's memory,
will also delete it again, and cause your bug.
The copy constructor must allocate a new Value just like your default
constructor does, so each object owns its own allocated memory.
| Quote: | I cannot actually free the momory associated with the result by calling a
destroy function, as return directly exits the function...
|
You shouldn't have a destroy function. You already have a destructor.
| Quote: | If I had to guess, you're making copies of TNumber and have a pointer
aliasing bug. Did you perhaps forget to implement the copy assignment
operator and the copy constructor?
probably not. how do I do this, and what exactly is the effect?
|
Whenever you create a temporary TNumber, you hit the problem described
above. The functions that take TNumber objects by value also cause
temporaries to be created. After fixing the constructor problem, the
crash should go away, but fixing the "by value" functions will then be
just a performance improvement.
Hope this helps.
--
Chris (TeamB); |
|
| Back to top |
|
|
Spine
Guest
|
| Posted: Wed May 16, 2007 8:11 am Post subject: Re: weird constructor - destructor problem |
|
|
Thanks a lot,
that did help. my BIG memory leak is gone now. I have made the copy
constructor exactly the same as the constructor. Does the compiler still
copy all the members into the new one? if so, when do you want your copy
constructor to be the same?
Spike
TNumber::TNumber(TNumber const & other)
{
Size = 25;
exp = 0;
sign=1;
Value = new int[Size];
for (int l=0;l<Size;l++) Value[l] = 0;
}
"Chris Uzdavinis (TeamB)" <chris (AT) uzdavinis (DOT) com> wrote in message
news:861whhspti.fsf (AT) explicit (DOT) atdesk.com...
| Quote: | "Spine" <boterkuipje (AT) hotmail (DOT) com> writes:
yes, I was affraid I did not provide enough information.
this is the declaration:
class TNumber
{
private:
int Size;
int *Value;
int exp;
int sign;
void Clean();
public:
TNumber();
~TNumber();
void Destroy();
void operator=(TNumber);
void operator=(double);
TNumber operator+(TNumber);
TNumber operator+(double);
TNumber operator-(TNumber);
TNumber operator-(double);
TNumber operator*(TNumber);
TNumber operator*(double);
TNumber operator/(TNumber);
TNumber operator/(double);
bool operator==(TNumber);
bool operator==(double);
bool operator<(TNumber);
AnsiString ToStr();
double ToDouble();
};
Ok, now for some "real" feedback. :)
Things you should change:
1) your operator=(TNumber) is taking TNumber by value, which is thus
constructed by the copy constructor, which is generated by the
compiler.
This should be taking a reference to a const TNumber:
void operator=(TNumber const &);
2) Your operator==(TNumber) is needlessly taking a copy of the
argument, rather than a reference to the original. This also
exacerbates the problem.
3) Your class lacks a copy constructor:
TNumber(TNumber const & other);
The problem is, when you make a copy by value, and you use the
compiler generated copy constructor, it copies all the members,
including the pointers. Thus, two objects will hold the same address
in its Value pointer. The first one that goes out of scope will
delete the memory. The second one, which aliases the other's memory,
will also delete it again, and cause your bug.
The copy constructor must allocate a new Value just like your default
constructor does, so each object owns its own allocated memory.
I cannot actually free the momory associated with the result by calling a
destroy function, as return directly exits the function...
You shouldn't have a destroy function. You already have a destructor.
If I had to guess, you're making copies of TNumber and have a pointer
aliasing bug. Did you perhaps forget to implement the copy assignment
operator and the copy constructor?
probably not. how do I do this, and what exactly is the effect?
Whenever you create a temporary TNumber, you hit the problem described
above. The functions that take TNumber objects by value also cause
temporaries to be created. After fixing the constructor problem, the
crash should go away, but fixing the "by value" functions will then be
just a performance improvement.
Hope this helps.
--
Chris (TeamB); |
|
|
| Back to top |
|
|
Chris Uzdavinis (TeamB)
Guest
|
| Posted: Wed May 16, 2007 4:15 pm Post subject: Re: weird constructor - destructor problem |
|
|
"Spine" <boterkuipje (AT) hotmail (DOT) com> writes:
| Quote: | Thanks a lot,
that did help. my BIG memory leak is gone now. I have made the copy
constructor exactly the same as the constructor. Does the compiler still
copy all the members into the new one? if so, when do you want your copy
constructor to be the same?
Spike
TNumber::TNumber(TNumber const & other)
{
Size = 25;
exp = 0;
sign=1;
Value = new int[Size];
for (int l=0;l<Size;l++) Value[l] = 0;
}
|
You should be copying all the values from "other" or else your new
object has no resemblance to the object of which it's supposed to be
copy.
Something like this:
TNumber::TNumber(TNumber const & other)
: Size(other.Size)
, exp(other.exp)
, sign(other.sign)
, Value(new int[other.Size]
{
memcpy( Value, other.Value, Size * sizeof(Value[0]) );
}
Also, ensure that the initialization list is in the same order as you
declare the members of your class.
To answer your question, no, if you write your own copy constructor,
then the compiler will not generate one for you or do anything
additional to what you wrote. That is, you have to do everything if
you do anything, when it comes to a copy constructor. I wrote the
above based on your example, assuming that no other member variables
also need initialization.
Don't forget operator=(TNumber const &) also needs to be carefully
written to ensure that the Value pointer isn't aliased, and that
things are properly handled.
--
Chris (TeamB); |
|
| Back to top |
|
|
Spine
Guest
|
| Posted: Wed May 16, 2007 8:35 pm Post subject: Re: weird constructor - destructor problem |
|
|
I feel stupid.
now it does not work any more. I have replaced the copy operator by the one
you showed.
TNumber::TNumber()
{
Size = 10;
exp = 0;
sign=1;
Value = new int[Size];
for (int l=0;l<Size;l++) Value[l] = 0;
}
TNumber::TNumber(TNumber const & other)
: Size(other.Size)
, Value(new int[other.Size])
, exp(other.exp)
, sign(other.sign)
{
memcpy( Value, other.Value, Size * sizeof(Value[0]) );
}
TNumber::~TNumber()
{
delete [] Value;
}
void TNumber::operator=(TNumber other)
{
Size = other.Size;
sign = other.sign;
exp = other.exp;
for (int l=0;l<Size;l++) Value[l] = other.Value[l];
Clean();
}
and in the header file:
class TNumber
{
private:
int Size;
int *Value;
int exp;
int sign;
void Clean();
public:
TNumber();
TNumber(TNumber const &);
~TNumber();
void operator=(TNumber);
when I delete the "delete [] Value" in the destructor it is working, but has
a major memory leak. If I put the "delete [] Value" in, it will hang itself.
I still need to pass the TNumbers by reference, but it should work like this
as well....
Spike
"Chris Uzdavinis (TeamB)" <chris (AT) uzdavinis (DOT) com> wrote in message
news:86tzudqasu.fsf (AT) explicit (DOT) atdesk.com...
| Quote: | "Spine" <boterkuipje (AT) hotmail (DOT) com> writes:
Thanks a lot,
that did help. my BIG memory leak is gone now. I have made the copy
constructor exactly the same as the constructor. Does the compiler still
copy all the members into the new one? if so, when do you want your copy
constructor to be the same?
Spike
TNumber::TNumber(TNumber const & other)
{
Size = 25;
exp = 0;
sign=1;
Value = new int[Size];
for (int l=0;l<Size;l++) Value[l] = 0;
}
You should be copying all the values from "other" or else your new
object has no resemblance to the object of which it's supposed to be
copy.
Something like this:
TNumber::TNumber(TNumber const & other)
: Size(other.Size)
, exp(other.exp)
, sign(other.sign)
, Value(new int[other.Size]
{
memcpy( Value, other.Value, Size * sizeof(Value[0]) );
}
Also, ensure that the initialization list is in the same order as you
declare the members of your class.
To answer your question, no, if you write your own copy constructor,
then the compiler will not generate one for you or do anything
additional to what you wrote. That is, you have to do everything if
you do anything, when it comes to a copy constructor. I wrote the
above based on your example, assuming that no other member variables
also need initialization.
Don't forget operator=(TNumber const &) also needs to be carefully
written to ensure that the Value pointer isn't aliased, and that
things are properly handled.
--
Chris (TeamB); |
|
|
| Back to top |
|
|
Chris Uzdavinis (TeamB)
Guest
|
| Posted: Wed May 16, 2007 10:33 pm Post subject: Re: weird constructor - destructor problem |
|
|
"Spine" <boterkuipje (AT) hotmail (DOT) com> writes:
| Quote: | I feel stupid.
now it does not work any more. I have replaced the copy operator by
the one you showed.
|
Can you define what "does not work" actually means?
| Quote: | void TNumber::operator=(TNumber other)
{
Size = other.Size;
sign = other.sign;
exp = other.exp;
for (int l=0;l<Size;l++) Value[l] = other.Value[l];
Clean();
}
|
This is NOT correct. How much memory does Value have? You have
overwritten Size, and now only assume that the sizes are equal. If
"other" has a larger Size, then you're going to walk off the end of
your array. Also, you should pass "other" by reference, and also
return a reference to *this, which is the conventional return value
from this operation.
Therefore, you need something like this:
TNumber &
TNumber::operator=(TNumber const & other)
{
int * newbuf = new int[other.Size];
memcpy(newbuf, other.Value, other.Size * sizeof(int));
Size = other.Size;
sign = other.sign;
exp = other.exp;
delete [] Value;
Value = newbuf;
return *this;
}
Note, if you're willing to add a swap member (usually a useful
function to have) you can greatly simplify your copy constructor code:
void
TNumber::swap(TNumber & other)
{
using std::swap; // be sure to #include <algorithm>
swap(Size, other.Size);
swap(sign, other.sign);
swap(exp, other.exp);
swap(Value, other.Value);
}
TNumber &
TNumber::operator=(TNumber const & other)
{
TNumber newbuf(other);
swap(other);
return *this;
}
Another question: What is "Clean()", what does it do, and why do you
call it from several places?
--
Chris (TeamB); |
|
| Back to top |
|
|
Spine
Guest
|
| Posted: Sun May 20, 2007 6:44 pm Post subject: Re: weird constructor - destructor problem |
|
|
| Quote: | now it does not work any more. I have replaced the copy operator by
the one you showed.
Can you define what "does not work" actually means?
yes. it means that the program will return a access violation error. read |
of adress 00000008. that cannot be good. the problem is that this does not
happen the first time I enter a specific routine, but it does always happen
at the same moment. (i.e. the program can use the class for a while, but the
error occurs at the same point in the program all the time)
| Quote: |
void TNumber::operator=(TNumber other)
{
Size = other.Size;
sign = other.sign;
exp = other.exp;
for (int l=0;l<Size;l++) Value[l] = other.Value[l];
Clean();
}
This is NOT correct. How much memory does Value have? You have
overwritten Size, and now only assume that the sizes are equal. If
"other" has a larger Size, then you're going to walk off the end of
your array. Also, you should pass "other" by reference, and also
return a reference to *this, which is the conventional return value
from this operation.
actually. At the moment Size is always the same. |
| Quote: |
Therefore, you need something like this:
TNumber &
TNumber::operator=(TNumber const & other)
{
int * newbuf = new int[other.Size];
memcpy(newbuf, other.Value, other.Size * sizeof(int));
Size = other.Size;
sign = other.sign;
exp = other.exp;
delete [] Value;
Value = newbuf;
return *this;
}
Note, if you're willing to add a swap member (usually a useful
function to have) you can greatly simplify your copy constructor code:
void
TNumber::swap(TNumber & other)
{
using std::swap; // be sure to #include <algorithm
swap(Size, other.Size);
swap(sign, other.sign);
swap(exp, other.exp);
swap(Value, other.Value);
}
TNumber &
TNumber::operator=(TNumber const & other)
{
TNumber newbuf(other);
swap(other);
return *this;
}
Another question: What is "Clean()", what does it do, and why do you
call it from several places?
Clean is actually something trivial. it rearranges the values that are in |
Value[], and does not create or delete any pointers.
this is what I have now: I have deleted as much code as posible, and still
the program will hang. In the main program I create an array of TNumber
using a pointer. (->could this be the problem? ) this works fine until at a
certain point the programs stops running. somehow this always happens in the
TNumber TNumber::operator+(TNumber const &other), where I create the new
TNumber even. (changing even to something else that surely does not exist
does not help).
TNumber::TNumber()
{
Size = 10;
exp = 0;
sign=1;
Value = new int[Size];
for (int l=0;l<Size;l++) Value[l] = 0;
}
TNumber::TNumber(TNumber const & other)
: Size(other.Size)
, Value(new int[other.Size])
, exp(other.exp)
, sign(other.sign)
{
memcpy( Value, other.Value, Size * sizeof(Value[0]) );
}
TNumber::~TNumber()
{
delete [] Value;
}
//---------------------------------------------------------------------------
TNumber& TNumber::operator=(TNumber const &other)
{
memcpy(Value,other.Value,other.Size * sizeof(int));
sign = other.sign;
exp = other.exp;
return *this;
}
TNumber& TNumber::operator=(double other)
{
if (other<0){
sign = -1;
other = -other;
}else sign = 1;
if (other!=0) exp = floor(log10(other));else exp=0;
AnsiString even = FloatToStr(other/pow(10,exp));
Value[0] = StrToFloat(even.SubString(1,1));
int tel=1;
while (tel<even.Length()-1){
Value[tel] = StrToInt(even.SubString(tel+2,1));
tel++;
}
for (int l=tel;l<Size;l++) Value[l]=0;
return *this;
}
//---------------------------------------------------------------------------
TNumber TNumber::operator+(TNumber const &other)
{
TNumber result = *this;
TNumber even;
return result;
}
TNumber TNumber::operator+(double other)
{
TNumber result,even;
even = other;
double sipke = ToDouble();
result = *this + even;
sipke = ToDouble();
AnsiString sipke1 = ToStr();
return result;
} |
|
| Back to top |
|
|
Chris Uzdavinis (TeamB)
Guest
|
| Posted: Mon May 21, 2007 4:32 pm Post subject: Re: weird constructor - destructor problem |
|
|
"Spine" <boterkuipje (AT) hotmail (DOT) com> writes:
| Quote: | this is what I have now: I have deleted as much code as posible, and still
the program will hang. In the main program I create an array of TNumber
using a pointer.
|
When you post code, though, it's helpful to always post enough that it
can be compiled and executed by a simple cut-and-paste. When you lave
out the class declaration, main(), and other parts of the prgram that
actually make use of the code, it's impossible to see the full
problem, or reproduce it ourselves;
| Quote: | (->could this be the problem? ) this works fine until at a
|
Anything could be a problem. Working with pointers is error-prone,
and since you are talking about accessing invalid memory, something is
clearly wrong.
| Quote: | certain point the programs stops running. somehow this always happens in the
TNumber TNumber::operator+(TNumber const &other), where I create the new
TNumber even. (changing even to something else that surely does not exist
does not help).
|
Hmmmm.
| Quote: | TNumber::TNumber()
{
Size = 10;
exp = 0;
sign=1;
Value = new int[Size];
for (int l=0;l<Size;l++) Value[l] = 0;
}
|
This constructor, like all constructors, would be better if it used
the initializer list whenever possible:
TNumber::TNumber()
: Size(10)
, exp(0)
, sign(1)
, Value(new int[Size])
{
memset( Value, '\0', Size * sizeof(Value[0]) );
}
Note very carefully that initializations are NOT done in the order of
the initializer list. The actual members are initialized in the order
they appear in the class declaration. Therefore, it is impererative
that you declare the member "Size" in your class before you declare
the Value array, or else this initialization won't work (since Value
is allocated in terms of Size, which obviously must be initialized
before it's used.)
I post this warning since the header isn't included, and I'm not going
to bother going back to old posts on the chance that what was posted
then is still the same today. (Another reason to include the
declarations in one post. A rule of thumb is: When posting code,
make it complete, and never use an #include for your own code. If the
file is self-sufficient to compile, then everyone can compile it
without having to have access to other files that may have not been
posted.)
| Quote: | TNumber& TNumber::operator=(TNumber const &other)
{
memcpy(Value,other.Value,other.Size * sizeof(int));
sign = other.sign;
exp = other.exp;
return *this;
}
|
This copy-assignment operator is still wrong. It is depending on the
fact that other's Size is the same, and will have undefined behavior
if the other's Size is larger. Do not depend on assumptions like
that, as it will eventually change. If the size must always be the
same amount for all instances, then it should not be a member
variable, and should be a constant value instead. (Such as an enum or
const static class member.)
| Quote: | TNumber& TNumber::operator=(double other)
{
if (other<0){
sign = -1;
other = -other;
}else sign = 1;
if (other!=0) exp = floor(log10(other));else exp=0;
AnsiString even = FloatToStr(other/pow(10,exp));
Value[0] = StrToFloat(even.SubString(1,1));
int tel=1;
while (tel<even.Length()-1){
Value[tel] = StrToInt(even.SubString(tel+2,1));
tel++;
}
for (int l=tel;l<Size;l++) Value[l]=0;
return *this;
}
|
Question: what is the max value that 'tel' can possibly have? How
does this code know that Value[tel] is actually indexing to a valid
value?
I see your for-loop more than once. Perhaps that indicates you should
have a member function named something like "zero_out_data()", which
factors out the implementation.
| Quote: | TNumber TNumber::operator+(TNumber const &other)
{
TNumber result = *this;
TNumber even;
return result;
}
|
Simply declaring even causes the problem? How about simply declaring
TNumber in main()... does that also have a problem?
Side note: the member functions like operator+ should be declared
"const" since they do not modify the current object.
I'm assuming the correctness of your operator+'s result is known to be
wrong, as you said you removed as much code as possible.
| Quote: | TNumber TNumber::operator+(double other)
{
TNumber result,even;
even = other;
double sipke = ToDouble();
result = *this + even;
sipke = ToDouble();
AnsiString sipke1 = ToStr();
return result;
}
|
There appears to be some useless work going on in here. For example,
there are no uses of sipke, yet it is assigned the result of ToDouble
twice, and sipke1 is also calculated but not used. Is this code
related to the problem, and you removed other lines of code that are
meaningful to the result but not related to the memory problem?
--
Chris (TeamB); |
|
| Back to top |
|
|
Spine
Guest
|
| Posted: Tue May 22, 2007 2:13 am Post subject: Re: weird constructor - destructor problem |
|
|
Thank you, thank you, thank you.
I found the problem.
| Quote: | Question: what is the max value that 'tel' can possibly have? How
does this code know that Value[tel] is actually indexing to a valid
value?
it doesn't. it allows tel to run above Size, and thus accesing Value[] that |
are not available, resulting in an error when I try to get more memory for
the next pointer. I think I need to do a complete security check on my
program (I didn't post it completely as it has 5 units, and you need all of
them.....)
thank you a lot for helping me out. I learned a lot in the process.
Sipke
"Chris Uzdavinis (TeamB)" <chris (AT) uzdavinis (DOT) com> wrote in message
news:86lkfiqumu.fsf (AT) explicit (DOT) atdesk.com...
| Quote: | "Spine" <boterkuipje (AT) hotmail (DOT) com> writes:
this is what I have now: I have deleted as much code as posible, and
still
the program will hang. In the main program I create an array of TNumber
using a pointer.
When you post code, though, it's helpful to always post enough that it
can be compiled and executed by a simple cut-and-paste. When you lave
out the class declaration, main(), and other parts of the prgram that
actually make use of the code, it's impossible to see the full
problem, or reproduce it ourselves;
(->could this be the problem? ) this works fine until at a
Anything could be a problem. Working with pointers is error-prone,
and since you are talking about accessing invalid memory, something is
clearly wrong.
certain point the programs stops running. somehow this always happens in
the
TNumber TNumber::operator+(TNumber const &other), where I create the new
TNumber even. (changing even to something else that surely does not exist
does not help).
Hmmmm.
TNumber::TNumber()
{
Size = 10;
exp = 0;
sign=1;
Value = new int[Size];
for (int l=0;l<Size;l++) Value[l] = 0;
}
This constructor, like all constructors, would be better if it used
the initializer list whenever possible:
TNumber::TNumber()
: Size(10)
, exp(0)
, sign(1)
, Value(new int[Size])
{
memset( Value, '\0', Size * sizeof(Value[0]) );
}
Note very carefully that initializations are NOT done in the order of
the initializer list. The actual members are initialized in the order
they appear in the class declaration. Therefore, it is impererative
that you declare the member "Size" in your class before you declare
the Value array, or else this initialization won't work (since Value
is allocated in terms of Size, which obviously must be initialized
before it's used.)
I post this warning since the header isn't included, and I'm not going
to bother going back to old posts on the chance that what was posted
then is still the same today. (Another reason to include the
declarations in one post. A rule of thumb is: When posting code,
make it complete, and never use an #include for your own code. If the
file is self-sufficient to compile, then everyone can compile it
without having to have access to other files that may have not been
posted.)
TNumber& TNumber::operator=(TNumber const &other)
{
memcpy(Value,other.Value,other.Size * sizeof(int));
sign = other.sign;
exp = other.exp;
return *this;
}
This copy-assignment operator is still wrong. It is depending on the
fact that other's Size is the same, and will have undefined behavior
if the other's Size is larger. Do not depend on assumptions like
that, as it will eventually change. If the size must always be the
same amount for all instances, then it should not be a member
variable, and should be a constant value instead. (Such as an enum or
const static class member.)
TNumber& TNumber::operator=(double other)
{
if (other<0){
sign = -1;
other = -other;
}else sign = 1;
if (other!=0) exp = floor(log10(other));else exp=0;
AnsiString even = FloatToStr(other/pow(10,exp));
Value[0] = StrToFloat(even.SubString(1,1));
int tel=1;
while (tel<even.Length()-1){
Value[tel] = StrToInt(even.SubString(tel+2,1));
tel++;
}
for (int l=tel;l<Size;l++) Value[l]=0;
return *this;
}
Question: what is the max value that 'tel' can possibly have? How
does this code know that Value[tel] is actually indexing to a valid
value?
I see your for-loop more than once. Perhaps that indicates you should
have a member function named something like "zero_out_data()", which
factors out the implementation.
TNumber TNumber::operator+(TNumber const &other)
{
TNumber result = *this;
TNumber even;
return result;
}
Simply declaring even causes the problem? How about simply declaring
TNumber in main()... does that also have a problem?
Side note: the member functions like operator+ should be declared
"const" since they do not modify the current object.
I'm assuming the correctness of your operator+'s result is known to be
wrong, as you said you removed as much code as possible.
TNumber TNumber::operator+(double other)
{
TNumber result,even;
even = other;
double sipke = ToDouble();
result = *this + even;
sipke = ToDouble();
AnsiString sipke1 = ToStr();
return result;
}
There appears to be some useless work going on in here. For example,
there are no uses of sipke, yet it is assigned the result of ToDouble
twice, and sipke1 is also calculated but not used. Is this code
related to the problem, and you removed other lines of code that are
meaningful to the result but not related to the memory problem?
--
Chris (TeamB); |
|
|
| Back to top |
|
|
Powered by phpBB © 2001, 2006 phpBB Group
.
|
FAQ
Search
Memberlist
Usergroups
Register
Profile
Log in to check your private messages
Log in
