Board index » cppbuilder » weird constructor - destructor problem

2007-05-16 03:20:55 AM
cppbuilder99
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
 

"Spine" < XXXX@XXXXX.COM >writes:
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);
 

"Chris Uzdavinis (TeamB)" < XXXX@XXXXX.COM >wrote in message
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...
Spike
 

{smallsort}

"Spine" < XXXX@XXXXX.COM >writes:
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.
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);
 

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)" < XXXX@XXXXX.COM >wrote in message

"Spine" < XXXX@XXXXX.COM >writes:
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);
 

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)" < XXXX@XXXXX.COM >wrote in message

"Spine" < XXXX@XXXXX.COM >writes:
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);
 

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)
Value[], and does not create or delete any pointers.
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;
}
 

"Spine" < XXXX@XXXXX.COM >writes:
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;
and since you are talking about accessing invalid memory, something is
clearly wrong.
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.)
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.)
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 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.
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);
 

Thank you, thank you, thank you.
I found the problem.
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)" < XXXX@XXXXX.COM >wrote in message

"Spine" < XXXX@XXXXX.COM >writes:
PS, please trim some of the quoted text when you post. For example,
there were 150 lines of quoted text in the last message that didn't
add any value to the post. Some people still use dial-up networking,
and it makes the archives larger than necessary, over-quoting is
distracting, etc.
Unless you're quoting program code and it's all *truly* necessary to
make your point, a good rule of thumb is to try to have no more quoted
lines than the number of lines you are writing. A 1-to-1 ratio (or
less) is desirable. Thanks!
--
Chris (TeamB);