class with destructor inside a vector...?

[ypjofficial]

Hello all,
I have encountered with following strange problem.
I am coding in C++ and using VC++ 6 compiler.

I have a class strvector containing char * cstr as a private member
and i have defined its destructor for releasing memory hold by cstr.

In main i created a vector<strvector>
and whenever i put a object of strvector inside this vector the program
crashes.

The complete program is as follows
/////////////////////////////////////////
#include <iostream.h>
#include "string"
#include "vector"

using namespace std;

class strvector
{
char * cstr;

public:

char * getcstr()
{
return cstr;
}
strvector(char * s)
{
cstr = new char[strlen(s)+1];
strcpy(cstr,s);
}
~strvector()
{
delete str;
}
/*release()
{
delete cstr;
}*/
};

void main()
{
vector<strvector> vec;

strvector str("one");
vec.push_back(str);//The program will crash
//str.release();

//cout<<vec[0].getcstr();//will print a garbage values.
}
///////////////////////////////////
If i remove the destructor and define a separate member function
release for releasing the memory,the cout statement will print the
garbase value.

Will someone please tell me what is going wrong in the above program
even if i am following a good programmering approach.(using destructor
etc.)?

Regards,
Yogesh Joshi

 

[Victor Bazarov]

strvector(char * s)
{
cstr = new char[strlen(s)+1];
strcpy(cstr,s);
}
~strvector()
{
delete str;

If you new[], you need to delete[].

}

Also, read about "the Rule of Three".

V

 

[ypjofficial]

If you new[], you need to delete[].
Still its crashing..Also i tried to put the copy constructor but no
improvement..still crashing..The code will run fine only when i remove
the destructor and will not release the memory held by cstr..

regards,
Yogesh

 

[John Harrison]

If you new[], you need to delete[].

Still its crashing..Also i tried to put the copy constructor but no
improvement..still crashing..The code will run fine only when i remove
the destructor and will not release the memory held by cstr..

regards,
Yogesh

The rule of three states that you need a destructor, copy constructor
and assignment operator.

If it is still crashing then post all the code. The bug is almost
certainly in the code for your copy constructor or assignment operator.

john

 

[ypjofficial]

John, as per your suggestion i tried to put copy constructor,
assignment operator and destructor..the problem got worst..it can't
even compile..
below is the code
////////////////////////////////// code starts
////////////////////////////////
#include <iostream.h>
#include "string"
#include "vector"

using namespace std;

class strvector
{
char * cstr;

public:
strvector()
{
}



strvector(strvector & s)
{
cstr = new char[strlen(s.cstr) + 1];
strcpy(cstr,s.cstr);
}


strvector(char * s)
{
cstr = new char[strlen(s)+1];
strcpy(cstr,s);
}

strvector & operator =(strvector & s)
{
cstr = new char[strlen(s.cstr) + 1];
strcpy(cstr,s.cstr);
return *this;

}

char * getcstr()
{
return cstr;
}

~strvector()
{
delete []cstr;
}
/* release()
{
delete cstr;
}
*/
};

void main()
{
vector<strvector> vec;

strvector str("one");
vec.push_back(str);
// str.release();

//cout<<vec[0].getcstr();
}
////////////////////////////////// code ends
//////////////////////////////////
and below are the compile errors

--------------------Configuration: Cpp2 - Win32
Debug--------------------
Compiling...
Cpp2.cpp
c:\program files\microsoft visual studio\vc98\include\xutility(39) :
error C2679: binary '=' : no operator defined which takes a right-hand
operand of type 'const class strvector' (or there is no acceptable
conversion)
c:\program files\microsoft visual studio\vc98\include\vector(170) : see
reference to function template instantiation 'void __cdecl
std::fill(class strvector *,class strvector *,const class strvector &)'
being compiled
c:\program files\microsoft visual studio\vc98\include\xmemory(34) :
error C2558: class 'strvector' : no copy constructor available
c:\program files\microsoft visual studio\vc98\include\xmemory(66) : see
reference to function template instantiation 'void __cdecl
std::_Construct(class strvector *,const class strvector &)' being
compiled
Error executing cl.exe.

Cpp2.obj - 2 error(s), 0 warning(s)


regards,
Yogesh Joshi

 

[Victor Bazarov]

John, as per your suggestion i tried to put copy constructor,
assignment operator and destructor..the problem got worst..it can't
even compile..

You need to get familiar with the concept of "const-correctness". See
my notes inline with the code.

below is the code
////////////////////////////////// code starts
////////////////////////////////
#include <iostream.h>
#include "string"
#include "vector"

using namespace std;

class strvector
{
char * cstr;

public:
strvector()
{
}



strvector(strvector & s)

strvector(strvector const & s)

{
cstr = new char[strlen(s.cstr) + 1];
strcpy(cstr,s.cstr);
}


strvector(char * s)

strvector(char const * s)

{
cstr = new char[strlen(s)+1];
strcpy(cstr,s);
}

strvector & operator =(strvector & s)

strvector & operator =(strvector const & s)

{
cstr = new char[strlen(s.cstr) + 1];
strcpy(cstr,s.cstr);
return *this;

}

char * getcstr()
{
return cstr;
}

~strvector()
{
delete []cstr;
}
/* release()
{
delete cstr;
}
*/
};

void main()

int main

{
vector<strvector> vec;

strvector str("one");
vec.push_back(str);
// str.release();

//cout<<vec[0].getcstr();
}
////////////////////////////////// code ends
> [..]

And spend some time reading the FAQ, it will help tremendously.

V

 

[John Harrison]

John, as per your suggestion i tried to put copy constructor,
assignment operator and destructor..the problem got worst..it can't
even compile..
below is the code
////////////////////////////////// code starts
////////////////////////////////
#include <iostream.h>
#include "string"
#include "vector"

Should be

#include <iostream>
#include <string.h>

using namespace std;

class strvector
{
char * cstr;

public:
strvector()
{
}

This is wrong because cstr will be garbage and you will end up deleteing
a garbage pointer. At least you should do

strvector()
{
cstr = 0;
}

strvector(strvector & s)

Should be,

strvector(const strvector & s)

{
cstr = new char[strlen(s.cstr) + 1];
strcpy(cstr,s.cstr);
}


strvector(char * s)
{
cstr = new char[strlen(s)+1];
strcpy(cstr,s);
}

strvector & operator =(strvector & s)

Should be

strvector & operator =(const strvector & s)

You cannot get by in C++ without understanding const.

{
cstr = new char[strlen(s.cstr) + 1];
strcpy(cstr,s.cstr);
return *this;

This leaks memory. Try this

if (&s != this)
{
delete[] cstr;
cstr = new char[strlen(s.cstr) + 1];
strcpy(cstr,s.cstr);
}
return *this;

}

char * getcstr()
{
return cstr;
}

~strvector()
{
delete []cstr;
}
/* release()
{
delete cstr;
}
*/
};

void main()
{
vector<strvector> vec;

strvector str("one");
vec.push_back(str);
// str.release();

//cout<<vec[0].getcstr();
}
////////////////////////////////// code ends
//////////////////////////////////

Now for me it compiles and runs without errors.

john

 

[Andre Kostur]

(e-mail address removed) wrote in @o13g2000cwo.googlegroups.com:

John, as per your suggestion i tried to put copy constructor,
assignment operator and destructor..the problem got worst..it can't
even compile..
below is the code

Comments inline:

////////////////////////////////// code starts
////////////////////////////////

#include <iostream.h>
#include "string"
#include "vector"

using namespace std;

class strvector
{
char * cstr;

public:
strvector()

: cstr(0) // Initialize the pointer to 0

{
}



strvector(strvector & s)

// This isn't a Copy Constructor (or at least
// not a normal one.... you should have:
strvector(const strvector & s)

{
cstr = new char[strlen(s.cstr) + 1];
strcpy(cstr,s.cstr);
}


strvector(char * s)
{
cstr = new char[strlen(s)+1];
strcpy(cstr,s);
}

strvector & operator =(strvector & s)
{
cstr = new char[strlen(s.cstr) + 1];
strcpy(cstr,s.cstr);
return *this;

}

char * getcstr()
{
return cstr;
}

~strvector()
{
delete []cstr;
}
/* release()
{
delete cstr;
}
*/
};

void main()
{
vector<strvector> vec;

strvector str("one");
vec.push_back(str);
// str.release();

//cout<<vec[0].getcstr();
}
////////////////////////////////// code ends
//////////////////////////////////
and below are the compile errors

--------------------Configuration: Cpp2 - Win32
Debug--------------------
Compiling...
Cpp2.cpp
c:\program files\microsoft visual studio\vc98\include\xutility(39) :
error C2679: binary '=' : no operator defined which takes a right-hand
operand of type 'const class strvector' (or there is no acceptable
conversion)
c:\program files\microsoft visual studio\vc98\include\vector(170) : see
reference to function template instantiation 'void __cdecl
std::fill(class strvector *,class strvector *,const class strvector &)'
being compiled
c:\program files\microsoft visual studio\vc98\include\xmemory(34) :
error C2558: class 'strvector' : no copy constructor available

As above.. you don't have a proper copy constructor...

 

[ypjofficial]

yes..i could compile the code and ran it without fail..
Thanks Victor,John and Adre!!!

Thanks and Regards,
Yogesh Joshi

 

[kamit]

Just out of curiosity. I tried to run the original code submitted by
Yogesh on Linux platform and it seems to work fine. I was wondering if
somebody could throw some light on that issue.

 

[kamit]

Just out of curiosity. I tried to run the original code submitted by
Yogesh on Linux platform and it seems to work fine. I was wondering if
somebody could throw some light on that issue.

 

[John Harrison]

Just out of curiosity. I tried to run the original code submitted by
Yogesh on Linux platform and it seems to work fine. I was wondering if
somebody could throw some light on that issue.

Yogesh's original code freed the same memory twice. The pointer str.cstr
would get freed twice, once when the vector was destroyed and once when
variable str was destroyed. This is because without a copy constructor
the str.cstr pointer would just be copied unchanged into the vector.
Adding a proper copy constructor fixed this problem.

But freeing the same memory twice does not necessarily mean that your
program will crash. It is an exampe of what C++ calls 'undefined
behaviour'. This means that C++ doesn't know how to handle the situation
and *anything* could happen. Yogesh saw a crash on his system, but you
didn't. That's OK, undefined behaviour means anything could happen.
Obviously if you want your programs to work you don't want undefined
behaviour, but just because you do have undefined behaviour doesn't mean
that your programs won't work. This is one thing that makes debugging
C++ tricky.

john

 

[kamit]

thanks for the feedback john. I really appreciate it.