overloading +=

[Luis]

hey guys, my assignment is to write a string class. below is all of my code.
When i try to overload the += operator, my program crashes every single
time. could some one check my code, and see what I am doing wrong please?
Thank You.

P.s. i would appreciate a promt response, i was due today, but if i finish
it soon my teacher might accept it. also if you have any ideas on how to
compare two c-strings using lexiographic ordeing please let me know.

#include <iostream>
#include "MyString.h"
#include <fstream>
#include <cassert>
#include <string>
using namespace std; //introduces namespace std

int main ( void )
{
MyString a("rr");
MyString b("aaa");
a+=b;
cout<<a;


return 0;
}


MyString::MyString()
{

string = "";
}

MyString::MyString(const char *inString)
{
string = new char[strlen(inString)+1];
strcpy(string,inString);
}


MyString::~MyString()
{
delete []string;
}

ostream& operator<<(ostream &out,const MyString &outString)
{
out<<outString.string;
return out;
}


MyString MyString:: operator=(MyString &right)
{
if (this != &right)
{
delete []string;
string = new char[strlen(right.string)+1];
strcpy(string,right.string);
}
return *this;
}


MyString::MyString(MyString &right)
{
string = new char[strlen(right.string)+1];
strcpy(string,right.string);
}


char& MyString:perator[](int index)
{
assert(index>=0 && index < strlen(string));
return string[index];
}


char MyString:perator[](int index)const
{
assert(index>=0 && index < strlen(string));
return string[index];
}


char *operator+(const MyString left,const MyString right)
{
char *temp;
temp = new char[strlen(left.string)+strlen(right.string)+1];
temp = strcat(left.string,right.string);
return temp;

}



istream& operator>>(istream& in,MyString &inString)
{

char temp[128];
delete [] inString.string;
in>>temp;
strcpy(inString.string,temp);

return in;
}


void MyString::read(istream &inString,char delimit)
{
char temp[128];
delete []string;
inString.getline(temp,127,delimit);
strcpy(string,temp);
}

char MyString:: operator+=(const MyString right)
{
MyString temp;
temp = *this;
delete []temp.string;
temp.string = new char[strlen(temp.string)+strlen(right.string)+1];
strcat(temp.string,right.string);
strcpy(string,temp.string);
return temp.string;


}


/* these are not working yet
bool operator<(const MyString left,const MyString right)
{

if(strcmp(left.string,right.string)<0)
return true;
}


bool operator>(const MyString left,const MyString right)
{

if(strcmp(left.string,right.string)>0)
return true;
}
*/


#ifndef MYSTRING_H
#define MYSTRING_H


#include <iostream>
using namespace std;

class MyString {
public:
//Constructors, Copy, and Destrctors

MyString(); //one of four
MyString(const char *inString); //two of four
MyString operator=(MyString &right); //three of four
MyString(MyString &right); //four of four
~MyString(); //destructor

//input/output
friend ostream& operator<<(ostream& out,const MyString &outString);
friend istream& operator>>(istream& in,MyString &inString);
void read(istream& inString,char delimit);
//Overloaded brackets
char& operator[](int index);
char operator[](int index)const;
//overloaded '+' & +=
char* operator+=(const MyString right);
friend char* operator+(const MyString left,const MyString right);
//Comparison operators


/*
friend bool operator<(const MyString left,const MyString right);
friend bool operator>(const MyString left,const MyString right);
*/
private:
operator>(const MyString left,const MyString right);
*/
};

#endif

 

[=?ISO-8859-1?Q?Juan_Antonio_Dom=EDnguez_P=E9rez?=]

int main ( void )

{
MyString a("rr");
MyString b("aaa");
a+=b;
cout<<a;

See forward notes, and you can see you are doing an access to
uninitilized memory, causing a segmentation fault.

return 0;
}


MyString::MyString()
{
string = "";
}

If you do this initializing of string, you cannot do delete[] string in
any other point of your code. You cannot delete an static "".

MyString::MyString(const char *inString)
{
string = new char[strlen(inString)+1];
strcpy(string,inString);
}


MyString::~MyString()
{
delete []string;

If you do Mystring* a=new MyString(); delete a; your program will crash
also.

}

ostream& operator<<(ostream &out,const MyString &outString)
{
out<<outString.string;
return out;
}



char& MyString:perator[](int index)
{
assert(index>=0 && index < strlen(string));
return string[index];
}


char MyString:perator[](int index)const
{
assert(index>=0 && index < strlen(string));
return string[index];
}


char *operator+(const MyString left,const MyString right)
{
char *temp;
temp = new char[strlen(left.string)+strlen(right.string)+1];
temp = strcat(left.string,right.string);
return temp;

}

don't Operator+ must return a MyString& ???

istream& operator>>(istream& in,MyString &inString)
{

char temp[128];
delete [] inString.string;
in>>temp;

Where are you allocating inString.string buffer? you delete it, but dont
realloc the new buffer.

strcpy(inString.string,temp);

return in;
}


void MyString::read(istream &inString,char delimit)
{
char temp[128];
delete []string;
inString.getline(temp,127,delimit);

Again string isnt a valid pointer. You dont allocate new buffer for string.

strcpy(string,temp);
}

char MyString:: operator+=(const MyString right)
{
MyString temp;
temp = *this;
delete []temp.string;
temp.string = new char[strlen(temp.string)+strlen(right.string)+1];
strcat(temp.string,right.string);

temp.string is reallocated, but string may have enough space or not. You
cannot strcpy from temp.string to string, because string is not a know
size buffer.

strcpy(string,temp.string);
return temp.string;


}


/* these are not working yet
bool operator<(const MyString left,const MyString right)
{

if(strcmp(left.string,right.string)<0)
return true;
}


bool operator>(const MyString left,const MyString right)
{

if(strcmp(left.string,right.string)>0)
return true;
}
*/


#ifndef MYSTRING_H
#define MYSTRING_H


#include <iostream>
using namespace std;

class MyString {
public:
//Constructors, Copy, and Destrctors

MyString(); //one of four
MyString(const char *inString); //two of four
MyString operator=(MyString &right); //three of four
MyString(MyString &right); //four of four
~MyString(); //destructor

//input/output
friend ostream& operator<<(ostream& out,const MyString &outString);
friend istream& operator>>(istream& in,MyString &inString);
void read(istream& inString,char delimit);
//Overloaded brackets
char& operator[](int index);
char operator[](int index)const;
//overloaded '+' & +=
char* operator+=(const MyString right);
friend char* operator+(const MyString left,const MyString right);
//Comparison operators


/*
friend bool operator<(const MyString left,const MyString right);
friend bool operator>(const MyString left,const MyString right);
*/
private:
operator>(const MyString left,const MyString right);
*/
};

#endif

 

[Karl Heinz Buchegger]

char *operator+(const MyString left,const MyString right)

Why does that one return a character pointer?
operator+, +=, * and all the other arithmetic operators usually
return either a reference or an object of the same class type they
are defined for.

{
char *temp;
temp = new char[strlen(left.string)+strlen(right.string)+1];
temp = strcat(left.string,right.string);
return temp;

} [snip]

char MyString:: operator+=(const MyString right)

Are you sure that the result of adding a MyString object to another
MyString object equals a single character?

{
MyString temp;
temp = *this;

creating a copy of *this ...

delete []temp.string;

deleting the characters stored in the temporary ...

temp.string = new char[strlen(temp.string)+strlen(right.string)+1];

accessing the previously deleted characters in the temporary.
Kaboom.

strcat(temp.string,right.string);

adding the characters from the 'right' string to a non existing
string in temp.string. temp.string just points at some memory, nothing
is stored there right now.
Kaboom.

strcpy(string,temp.string);
return temp.string;

}

Also: usually arguments to operators are passed by const reference.
So a typical framework for your operators would look like this:

MyString operator+( const MyString& left, const MyString& right )
{
MyString result;
...
return result;
}

MyString& MyString:perator+=( const MyString& right )
{
...
return *this;
}

Note the difference: operator+ creates a temporary which will hold the result.
Thus you can't return by reference, you have to return by value.
operator+= on the other hand works on the object it is called for. This object
is still around after the function finishes, thus operator+= can return a reference.

 

[Peter van Merkerk]

hey guys, my assignment is to write a string class. below is all of my
code.

When i try to overload the += operator, my program crashes every single
time. could some one check my code, and see what I am doing wrong please?
Thank You.

P.s. i would appreciate a promt response,

I you want prompt responses post minimal, yet complete and compilable
code that demonstates the problem. The easier it is for people to
reproduce the problem you are having, the more likely you get prompt
responses. The code you posted is not compilable, and has alot of stuff
that is not relevant to the problem you are having.

i was due today, but if i finish
it soon my teacher might accept it. also if you have any ideas on how to
compare two c-strings using lexiographic ordeing please let me know.

#include <iostream>
#include "MyString.h"
#include <fstream>
#include <cassert>
#include <string>
using namespace std; // introduces namespace std

Redundant comment, don't explain standard language constructs with
comments.

int main ( void )
{
MyString a("rr");
MyString b("aaa");
a+=b;
cout<<a;

return 0;
}

MyString::MyString()
{

string = "";
}

This is going to give problems when you do 'delete[] string;' since
delete[] may only be done on objects allocated with new[]. This is
probably also explains the problem you are having with operator+= as
that function uses a default constructed MyString.

A quick fix would be to rewrite the default constructor like this:

MyString::MyString()
{
string = new char[1];
string[0] = 0;
}

MyString::MyString(const char *inString)
{
string = new char[strlen(inString)+1];
strcpy(string,inString);
}


MyString::~MyString()
{
delete []string;
}

ostream& operator<<(ostream &out,const MyString &outString)
{
out<<outString.string;
return out;
}

MyString MyString:: operator=(MyString &right)
{
if (this != &right)
{
delete []string;
string = new char[strlen(right.string)+1];
strcpy(string,right.string);
}
return *this;
}

Usually assignment operators return a reference to themselves
(MyString&) instead of a copy.

MyString::MyString(MyString &right)
{
string = new char[strlen(right.string)+1];
strcpy(string,right.string);
}

Copy constructors usually take a const reference (const MyString &)
rather than a reference to the object to be copied, because the object
to be copied should not be modified.

char& MyString:perator[](int index)
{
assert(index>=0 && index < strlen(string));
return string[index];
}

char MyString:perator[](int index)const
{
assert(index>=0 && index < strlen(string));
return string[index];
}

char *operator+(const MyString left,const MyString right)
{
char *temp;
temp = new char[strlen(left.string)+strlen(right.string)+1];
temp = strcat(left.string,right.string);
return temp;
}

Don't return a char pointer, this creates a memory leak:

MyString a("Memory");
MyString b("Leak!!!");
MyString c(a+b); // Memory allocated by operator+ will not be
released!

operator+ should return a MyString object. It is also wise to use const
references (const MyString &) for the input parameters.

istream& operator>>(istream& in,MyString &inString)
{

char temp[128];
delete [] inString.string;
in>>temp;
strcpy(inString.string,temp);

return in;
}

This is going to fail miserably when more than 128 characters come from
the 'in' stream.

void MyString::read(istream &inString,char delimit)
{
char temp[128];
delete []string;
inString.getline(temp,127,delimit);
strcpy(string,temp);
}

char MyString:: operator+=(const MyString right)

It doesn't make sense to return just a char (it also mismatches with the
header file you posted);
return a reference to self (MyString&) instead

{
MyString temp;
temp = *this;
delete []temp.string;

See my comment I made for the default constructor; you are deleting
something here that has not been allocated with new[]!!!

temp.string = new char[strlen(temp.string)+strlen(right.string)+1];
strcat(temp.string,right.string);
strcpy(string,temp.string);
return temp.string;
}

Since you only using the string member of the temp object, you might
just as well use a local char* variable instead. Using a temporary
MyString object has absolutely no added value here, it only complicates
things.

#ifndef MYSTRING_H
#define MYSTRING_H


#include <iostream>
using namespace std;

Never put a using statement in a header file like this. Everyone
including this header file pulls the std namespace into the current
namespace without knowing it. This can lead to very difficult to track
down problems.

<snip>