The core of what's gone wrong in the question's operator= implementation is that it does no assigning to the destination object. Let's break down what happens:
const str& str::operator=(const str& s)
{
str a; // make a temporary
a._n = s.length(); // assign source's length to temporary's length
a._buf = new char[a._n]; // assign a buffer to the temporary
strcpy(a._buf, s._buf); // assign source's data to temporary
return s; // return source
}; // temporary goes out of scope and is destroyed
Since everything was stored in the temporary a and not into this, after dest = source; dest comes away unchanged. All of the work done was lost when a went out out of scope at the end of the function and was destroyed.
To fix this up:
The return of the source is a bit strange, but you'll probably never notice the effects once the rest is fixed. Idiomatically, you should return the object that was just assigned. Really good reading on what operator= (and all of the common operator overloads) should look like can be found here: What are the basic rules and idioms for operator overloading? It's simple and easy to get out of the way, so we'll make that our first correction before moving on to the main event.
str& str::operator=(const str& s)
{
str a;
a._n = s.length();
a._buf = new char[a._n];
strcpy(a._buf, s._buf);
return *this;
};
Note the guts of this function look nothing like what sbi suggests. sbi is using the Copy and Swap approach to assignment operators. We'll touch back on this a bit later because it is often an excellent place to start and almost as often an excellent place to stop because it is just so simple that it is almost impossible to get wrong.
Creating a temporary, a, gives you something other than the intended destination, this, to assign to, so let's just get rid of a and use this.
const str& str::operator=(const str& s)
{
this->_n = s.length();
this->_buf = new char[this->_n];
strcpy(this->_buf, s._buf);
return *this;
};
But we don't need to explicitly state this everywhere, so
const str& str::operator=(const str& s)
{
_n = s.length();
_buf = new char[_n];
strcpy(_buf, s._buf);
return *this;
};
Now the destination object has been assigned a copy of everything in the source, but what if there was stuff in the destination already? Whoops. Just leaked it. Assuming an empty str is nullptr or provided with a valid allocation, the solution to this problem is a quick one:
const str& str::operator=(const str& s)
{
delete[] _buf;
_n = s.length();
_buf = new char[_n];
strcpy(_buf, s._buf);
return *this;
};
But what if you're doing something silly like a = a and assigning the object to itself?
const str& str::operator=(const str& s)
{
delete[] _buf; // same object so this deleted s._buf
_n = s.length();
_buf = new char[_n];
strcpy(_buf, s._buf); // and s._buf is gone, replaced by the new empty _buf
// bad stuff will happen here
return *this;
};
You could simply say, "That's on you, fool." and carry on. There's plenty of that thinking in the C++ standard library so you wouldn't be out of line, but if you want to be forgiving
const str& str::operator=(const str& s)
{
if (this != &s)
{
delete[] _buf;
_n = s.length();
_buf = new char[_n];
strcpy(_buf, s._buf);
}
return *this;
};
Now we have something that won't embarrass us. Unless s.length() doesn't include the terminating null that allows strcpy to function correctly. Traditionally string length functions do NOT include the terminating null, so we need an extra character to fit in the null terminator. This is the most likely cause of the crash, by the way.
const str& str::operator=(const str& s)
{
if (this != &s)
{
delete[] _buf;
_n = s.length();
_buf = new char[_n + 1]; // +1 for null terminator
strcpy(_buf, s._buf);
}
return *this;
};
Note that you will need this extra byte everywhere you assign a buffer to _buf.
Back to Copy and Swap, sort of.
You need a copy constructor to implement the Copy part of Copy and Swap, but to observe The Rule of Three the object needs to have a copy constructor anyway. Let's take a look at what we need in order to implement one.
str::str(const str& s)
{
if (this != &s) // don't need because you have to be really stupid to copy yourself
{
delete[] _buf; // don't need because this is a new object. It can't have an
// existing allocation
_n = s.length();
_buf = new char[_n + 1];
strcpy(_buf, s._buf);
}
};
It is possible to
str a(a);
and construct an object as a copy of its incompletely constructed self. I don't really know why this is legal in C++. Probably because the rules to prevent it would have screwed up something else more important. Doing this is kind of epic-stupid. a = a; can happen because of some confusing indirection where what you actually wrote was a = b;, but a few functions back b was initialized as a reference to a. Or maybe you're working with pointers and screwed up. Anyway, it can happen. str a(a); requires either some deliberation or a typo. Either way it's flat out wrong and should be fixed. I don't recommend trying to protect against this case.
Anyway, we can simplify the copy constructor to
str::str(const str& s)
{
_n = s.length();
_buf = new char[_n + 1];
strcpy(_buf, s._buf);
};
by removing the stuff we don't need. And then, through the magic of the Member Initializer List, we can reduce the constructor to
str::str(const str& s): _n(s._n), _buf(new char[_n + 1])
{
strcpy(_buf, s._buf);
};
And with this beauty in place you can take advantage of Copy and Swap
str& str::operator=(str s) // copy constructor does all the copying
{
std::swap(_n, s._n); // exchange guts with s
std::swap(_buf, s._buf);
return *this;
}; // destruction of s deletes this's old _buf
or this, if you have implemented your own swap function for swapping strs.
str& str::operator=(str s)
{
swap(*this, s);
return *this;
};
It's very handy to have a swap function if you will be doing sorting of strs later.
Pheeew! Now let's look at operator+
str operator+(const str& a, const str& b)
{
str c ;
c._n = a._n + b._n;
c._buf = new char[c._n]; // need a +1 here for the null terminator
strcpy(c._buf,a._buf);
strcat(c._buf ,b._buf);
//strcat(strcat(c._buf,""),b._buf);
return c;
}
This guy's not bad. Needs some extra space for the terminator (c._buf = new char[c._n+1];) but otherwise serviceable. You can probably stop here.
But if you need to also implement operator+= for the assignment, you might as well go with the idiomatic solution. What are the basic rules and idioms for operator overloading? recommends implementing operator+= and basing operator+ around it. That would look something like
str& str::operator+=(const str& b)
{
_n += b._n;
char * temp = new char[_n+1]; // can't use _buf yet. Need free it's memory and
// to copy from it
strcpy(temp, _buf);
strcat(temp, b._buf);
delete[] _buf; // now we can release and reuse _buf
_buf = temp;
return *this;
}
str operator+(str a, const str& b)
{
a+=b;
return a;
}
A quick note on Idioms:
In general, one should stick to the idiomatic solutions unless there is a compelling reason not to. They are immediately recognizable to an experienced programmer and require no explanation. This saves time in code reviews and writing documentation and results in fewer surprises when other people work on or with your code. Most importantly they work. They may not work as well as a specialized solution, for example the overhead of the Copy and Swap Idiom can be unnecessarily expensive, but they're a great place to start until it's demonstrated that there is a better solution AND you have a good reason to take advantage of that other solution.
