how to improve custom String implementation

[Dev]

Hello,

In the following class definition,
the ZString destructor is invoked two times.
This crashes the code.

class ZString
{
public:
ZString(char* p)
: _sz(::strlen(p)),
_p(new char[_sz+1])
{
::strcpy(_p,p);
}

~ZString()
{
delete [] _p;
}

char& operator [] (int i)
{
return _p[i];
}

private:
int _sz;
char* _p;
};
int main(int argc, char* argv[])
{
ZString s1("John");
ZString s2 = s1;

return 0;
}

An immediate solution that comes to my mind is to provide
the implementation of the assignment operator, wherein a
new ZString object is created using the field values from the
existing ZString object (rhs).

Can anybody suggest better approaches to improve the class
implementation, with trade offs ?

Thanks in advance.

Dev.

[Rolf Magnus]

Dev wrote:

Hello,

In the following class definition,
the ZString destructor is invoked two times.
Well, there are two ZString objects. Each of them gets constructed and each
of them gets destroyed.
This crashes the code.

class ZString
{
public:
ZString(char* p)
: _sz(::strlen(p)),
_p(new char[_sz+1])
{
::strcpy(_p,p);
}

~ZString()
{
delete [] _p;
}

char& operator [] (int i)
{
return _p[i];
}

private:
int _sz;
char* _p;
};
int main(int argc, char* argv[])
{
ZString s1("John");
ZString s2 = s1;

return 0;
}

An immediate solution that comes to my mind is to provide
the implementation of the assignment operator, wherein a
new ZString object is created using the field values from the
existing ZString object (rhs).
In your code, the assignment operator is not involved, but rather the copy
constructor. An assignment operator takes an existing objects and replaces
its value with that of an other object.
Can anybody suggest better approaches to improve the class
implementation, with trade offs ?

No, the implementation of an assignment operator and a copy constructor is
exactly what you need. Both of them get automatically created by the
compiler if you don't provide your own implementation. Those just do a
memberwise copy, which in your case is not what you want.
The problem is that the second object gets created through the copy
constructor. The pointer member gets copied, so both objects' pointers
point to the same memory location. When one of your objects is destroyed,
the memory gets freed, but the other object still expects the pointer to be
valid.
So you need to provide your own implementation that does The Right
Thing(tm).
There is a rule called the Rule of Three that says: If you need one of a
copy constructor, assignment operator and destructor, you probably (almost
definitely) need all three of them.
You just found out the reason for that rule.

[John Fullman]

Yup.. they are right.. You need one of these

ZString(const ZString& s)
: _sz(s._sz),
_p(new char[_sz+1])
{
::strcpy(_p,s._p);
}

[jois.de.vivre]

Dev wrote:

Hello,

In the following class definition,
the ZString destructor is invoked two times.
This crashes the code.

class ZString
{
public:
ZString(char* p)
: _sz(::strlen(p)),
_p(new char[_sz+1])
{
::strcpy(_p,p);
}

~ZString()
{
delete [] _p;
}

char& operator [] (int i)
{
return _p[i];
}

private:
int _sz;
char* _p;
};
int main(int argc, char* argv[])
{
ZString s1("John");
ZString s2 = s1;

return 0;
}

An immediate solution that comes to my mind is to provide
the implementation of the assignment operator, wherein a
new ZString object is created using the field values from the
existing ZString object (rhs).

Can anybody suggest better approaches to improve the class
implementation, with trade offs ?

Thanks in advance.

Dev.

If you haven't defined a copy constructor, the compiler does this for
you. The problem is that it does a shallow copy, so both your objects
have the same address for the _p pointer. When your main() returns, it
deallocates __p for each object, and thus causing a double free. You
have to define the copy constructor where you explicitly allocate
memory for _p and do a strcpy.

[Dev]

Dev wrote:

Hello,

In the following class definition,
the ZString destructor is invoked two times.
This crashes the code.

class ZString
{
public:
ZString(char* p)
: _sz(::strlen(p)),
_p(new char[_sz+1])
{
::strcpy(_p,p);
}
I added an copy constructor here.


int main(int argc, char* argv[])
{
ZString s1("John");
ZString s2 = s1;

return 0;
}

Additionally, I set s1 = 0
immediately after construction of s2.

Now, I get a Segmentation Fault.

Is there a way to get around this problem ?

thanks
Dev.

[John Harrison]

Dev wrote:

Dev wrote:

Hello,

In the following class definition,
the ZString destructor is invoked two times.
This crashes the code.

class ZString
{
public:
ZString(char* p)
: _sz(::strlen(p)),
_p(new char[_sz+1])
{
::strcpy(_p,p);
}

I added an copy constructor here.

int main(int argc, char* argv[])
{
ZString s1("John");
ZString s2 = s1;

return 0;
}

Additionally, I set s1 = 0
immediately after construction of s2.

Now, I get a Segmentation Fault.

Is there a way to get around this problem ?

Of course, what sort of language do you think C++ is?

You should stop focussing on the two calls to the destructor, that is
correct, it is what happens before that that is incorrect. Since you
haven't posted the copy constructor you wrote I would expect that the
error is there.

You are also missing an operator= for you class, which is essential in
the long run, but should not be causeing the problems above. It's a
common situation in C++ that when yu program crashes, it not the code at
the point it crashes that is wrong but the code somwhere before where it
crashes.

I don't understand what you mean by 'Additionally, I set s1 = 0' does
that mean that you program was literally like this

int main(int argc, char* argv[])
{
ZString s1("John");
ZString s2 = s1;
s1 = 0;
return 0;
}

If that is the case, then I don't understand why you are surprised you
got a crash. It's an obvious null pointer error.

Here is a working ZString class and program. The main changes from you
code are the addition of const in the appropriate places, a working
assignment operator and copy constructor and a const version of
operator[]. All these things are necessary for a simple string class.

#include <cstring>
#include <algorithm>

class ZString
{
public:
ZString(const char* p)
: _sz(::strlen(p)),
_p(new char[_sz+1])
{
::strcpy(_p,p);
}

ZString(const ZString& rhs)
: _sz(::strlen(rhs._p)),
_p(new char[_sz+1])
{
::strcpy(_p, rhs._p);
}

void swap(ZString& rhs)
{
std::swap(_sz, rhs._sz);
std::swap(_p, rhs._p);
}

ZString& operator=(const ZString& rhs)
{
ZString tmp(rhs);
swap(tmp);
return *this;
}

~ZString()
{
delete [] _p;
}

char& operator [] (int i)
{
return _p[i];
}

char operator [] (int i) const
{
return _p[i];
}

private:
int _sz;
char* _p;
};
int main(int argc, char* argv[])
{
ZString s1("John");
ZString s2 = s1;

return 0;
}

john

[Karl Heinz Buchegger]

John Harrison wrote:

I don't understand what you mean by 'Additionally, I set s1 = 0' does
that mean that you program was literally like this

int main(int argc, char* argv[])
{
ZString s1("John");
ZString s2 = s1;
s1 = 0;
return 0;
}

If that is the case, then I don't understand why you are surprised you
got a crash. It's an obvious null pointer error.

[] class ZString
{
public:
ZString(const char* p)
Watch out for a passed a 0 pointer as the OP did
: _sz(::strlen(p)),
_p(new char[_sz+1])
{
::strcpy(_p,p);
}

--
Karl Heinz Buchegger
kb******@gascad.at

[Rolf Magnus]

Karl Heinz Buchegger wrote:

John Harrison wrote:

>

I don't understand what you mean by 'Additionally, I set s1 = 0' does
that mean that you program was literally like this

int main(int argc, char* argv[])
{
ZString s1("John");
ZString s2 = s1;
s1 = 0;
return 0;
}

If that is the case, then I don't understand why you are surprised you
got a crash. It's an obvious null pointer error.

[]

class ZString
{
public:
ZString(const char* p)

Watch out for a passed a 0 pointer as the OP did

: _sz(::strlen(p)),
_p(new char[_sz+1])
{
::strcpy(_p,p);
}

For example like this:

ZString(const char* p)
: sz_(p ? std::strlen(p) : 0),
p_(new char[sz_+1])
{
if (p)
std::memcpy(p_,p, sz_);
else
*p_ = '\0';
}

I chose trailing undescores, because they are not reserved, I used the
functions from the std namespace, because they are supposed to be
preferred, and I used memcpy instead of strcpy, because that might be a bit
faster and has no disadvantage.