operator+ overload, simple string class 3

[Skute]

Hi,

im trying to overload the "+" operator to join 3 strings together, but im having difficulty. Im only doing this for a bit of experimentation.

CStr String1 = "Part1";
CStr String2 = "Part2\n";
CStr String3;
String3 = String1 + " " + String2;

// This works...
//String3.operator+(String1);
//String3.operator+(" "
//String3.operator+(String2);
printf(String3);


CStr:perator char*()
{
return _str;
}

CStr& CStr:perator=(const char* String)
{
this->Copy(String);
return *this;
}

CStr& CStr:perator+(const char* String)
{
this->Join(String);
return *this;
}


However, this is bad because, when using the "+" operator, like this:

CStr String1 = "Part1";
CStr String2 = "Part2\n";
CStr String3;
String3 = String1 + " " + String2;

String1 ends up with the value of "Part1 ", instead of "Part1". And also the program crashes when the classes are deleted because it automatically frees the memory. but because of the return *this, its messing up.

Anyone got any pointers for this kind of thing?
cheers

Skute

"There are 10 types of people in this World, those that understand binary, and those that don't!"

 

[ArkM]

What's your

CStr::Copy()

and

CStr::Join()

? I don't see them...
Why do you write

this->Copy(String)

instead of simple

Copy(String)

? Where is common assignment protector

if (this != &String)

?
If you don't use stl::string and want to define Yet_Another_String class, show us sources of troubles...

 

[Skute]

Didnt think there was much point in showing you copy functions...

I use, this->Function() because i think it looks clearer, helps me when i look in the future because i quickly know im calling an internal function.

void CStr::Copy(const char* Source)
{
this->Clear();

int iMemSize = 0;
iMemSize = sizeof(char) * (int)strlen(Source) + 1;
_str = (char *)malloc(iMemSize);
memset(_str, 0, iMemSize);
strcpy(_str, Source);
}

void CStr::Join(const char* String)
{
int iMemSize = 0;

iMemSize = (int)strlen(_str) + (int)strlen(String) + 1;
_str = (char *)realloc(_str, iMemSize);

strcpy(_str + strlen(_str), String);
}

void CStr::Clear()
{
if (_str != NULL)
{
free(_str);
_str = NULL;
}
}

Skute

"There are 10 types of people in this World, those that understand binary, and those that don't!"

 

[ArkM]

CStr String3; // Empty CStr object. What's about _str?
String3 = ... // Join() invokes strlen(this->_str) - str==?

It seems we need a CStr::CStr() definition...
You Copy() crashes old _str pointer value: memory leak occurs...
Note abote a common practice: don't mix C-malloc and C++ new op allocations...
Check also

CStr::~Cstr()

destructor. It plays here too (in ...+...+... expression).

 

[Skute]

My current implementation of operator+ is:

CStr CStr:perator+(const char* String)
{
CStr temp;

temp = this->ToString();
temp += String;

return temp;
}

I have removed this->Clear() from the CStr::~CStr() function, so now whenever i use a string i get left with a memory leak. Is there any way to counter that? (Other than calling CStr::Clear() at the end of each function i use a string in?)

Skute

"There are 10 types of people in this World, those that understand binary, and those that don't!"

 

[ArkM]

Check carefully all CStr implementation. If one allocate memory, anybody must free it in a proper time. Draw life state graph for _str, for example. You CStr is not a very complex class, you may play all common use patterns for its objects.
Be careful: your operator+() needs for explicit copy constructor (and operator-(), of course) to create new copy for _str member. Default copy constructor (copy members) is wrong: you have dynamically allocated _str, for example. If you simply move this pointer into another CStr object, you will get two pointers to one memory chunk in the heap! Free it via one - another refers to the free memory slot...
Don't worry about special instruments: check the code on the screen (or better print it) step by step then run with VS debugger...

 

[Skute]

but i have overloaded the operator= as well?

String& String:perator=(const char* Source)
{
if (Source == NULL)
this->Clear();
else
this->Copy(Source);

return *this;
}

String& String:perator=(String& Source)
{
this->Copy(Source.ToString());
return *this;
}


So you can see that im not creating duplicate pointer entries, im actually copying the char data...?

The class currently works, its just the memory is never free'd.

Skute

"There are 10 types of people in this World, those that understand binary, and those that don't!"

 

[PerFnurt]

To get all functionality in place you could start by letting CStr simply encapsulate a std::string.

Make tests that concats strings and whatnot. When you've got it to work, replace std::string with your own data management implementation (nothing prevents that from being a class of its own) CStrData or something...

When you've got that to work, you'll prolly think "Hmmmm...how can I optimize this" and then you focus on CStrData to handle multiple strings of the same content (like std::string and CString does). This is just me guessing...

Of course, since you've kept the previous mentioned tests you can always check that you don't break anything.

Btw, new is nice. malloc is...well...malloc.



/Per

_{"It was a work of art, flawless, sublime. A triumph equaled only by its monumental failure."}

 

[Skute]

are people against using malloc?

hmm, thanks for points Perf, how do you mean "multiple strings of the same content"?

thanks

Skute

"There are 10 types of people in this World, those that understand binary, and those that don't!"

 

[PerFnurt]

Well

CStr s("Foo"
CStr s2 = s;

s2 and s could share the same data for "Foo"...

/Per

_{"It was a work of art, flawless, sublime. A triumph equaled only by its monumental failure."}

 

[ArkM]

Dear Scute, sometimes you look like a steady partisan under cross-examination (;-). If your current implementation of operator+ uses ToString(), what's this new player ToString() mission? I can't compile your current implementation: no CStr:perator+=(const CStr&).
Where is CStr constructors? Where is ~CStr()? You can't use a default constructor for the class with dynamic allocated member, you must define copy constructor CStr(const CStr&) or you have 100% memory leaks or worst case.

 

[Skute]

The current implementation stands like this, and yes, there are terrible memory leaks in it. Because im allocating memory for each string object and never reclaiming it:



#include <stdlib.h>
#include <string.h>
#include <malloc.h>
#include &quot;String.h&quot;

String::String()
{
_str = NULL;
Copy(&quot;&quot;
}

String::String(char* Source)
{
if (Source == NULL)
this->ThrowException(NULLPOINTER);
_str = NULL;
Copy(Source);
}

String::~String()
{
//this->Clear();
}

void String::Copy(const char* Source)
{
if (Source == NULL)
this->ThrowException(NULLPOINTER);

this->Clear();

int iMemSize = 0;
iMemSize = sizeof(char) * (int)strlen(Source) + 1;
_str = (char *)malloc(iMemSize);

if (_str == NULL)
this->ThrowException(MALLOCFAILED);

memset(_str, 0, iMemSize);
strcpy(_str, Source);
}

void String::Join(const char* Source)
{
int iMemSize = 0;

iMemSize = (int)strlen(_str) + (int)strlen(Source) + 1;
_str = (char *)realloc(_str, iMemSize);

if (_str == NULL)
this->ThrowException(MALLOCFAILED);

strcpy(_str + strlen(_str), Source);
}

void String::Clear()
{
if (_str != NULL)
{
try
{
free(_str);
_str = NULL;
}
catch (...)
{
this->ThrowException(FREEFAILED);
}
}
}

const char* String::ToString()
{
return _str;
}

int String::Size()
{
if (_str == NULL)
return -1;

return (int)strlen(_str);
}

String:perator char*()
{
return _str;
}

String& String:perator=(const char* Source)
{
if (Source == NULL)
this->Clear();
else
this->Copy(Source);

return *this;
}

String& String:perator=(String& Source)
{
this->Copy(Source.ToString());
return *this;
}

String String:perator+(const char* Source)
{
if (Source == NULL)
this->ThrowException(NULLPOINTER);

String temp;

temp = this->ToString();
temp += Source;

return temp;
}

String String:perator+(String& Source)
{
String temp;

temp = this->ToString();
temp += Source.ToString();

return temp;
}

String& String:perator+=(const char* Source)
{
if (Source == NULL)
this->ThrowException(NULLPOINTER);

this->Join(Source);
return *this;
}

String& String:perator+=(String& Source)
{
this->Join(Source.ToString());
return *this;
}

bool String:perator!=(String& Source)
{
if (strcmp(this->ToString(), Source.ToString()) == 0)
return false;
else
return true;
}

bool String:perator==(String& Source)
{
if (strcmp(this->ToString(), Source.ToString()) == 0)
return true;
else
return false;
}

void String::ThrowException(StringExceptionType Type, const char* Message)
{
StringException ex(Type, Message);
throw ex;
}



(You will have to comment out the throwexception lines because i havent included that class)



Skute

&quot;There are 10 types of people in this World, those that understand binary, and those that don't!&quot;

 

[PerFnurt]

Was that a question or just info?

There is something fishy about possibly throwing an exception from the Clear, since you'd want to clear the stuff in the destructor (though I note you've commented it out). Note that a destructor may never-never-never throw an exception...

/Per

_{&quot;It was a work of art, flawless, sublime. A triumph equaled only by its monumental failure.&quot;}

 

[Skute]

Yeah, the reason its commented out is because when i do something such as:


String3 = String1 + &quot; &quot; + String2;


It crashes because the temp variable that gets returned has had the data inside it cleared. Try running the code with // this->Clear() uncommented in the deconstructor.

Skute

&quot;There are 10 types of people in this World, those that understand binary, and those that don't!&quot;

 

[ArkM]

Again and again... You MUST define your own copy constructor String(const String&). Default copy constructor is the main source of your troubles. Here is its (dft ctor) 'code':

String(const String& s) { _str = s._str; }
[code]
That's all: so beloved [code]this->_str

killed with its char array in the heap...
C++ uses copy constructor when evaluates your test expression

s3 = s1 + " " + s2

. It uses wrong (in your case) default copy constructor! Read any C++ book about copy constructor role. Place String(const String&); declaration into the private part of String: you will get linkage error (no copy ctor definition).

I will try to examine your full code later. It seems there are some defects in the code. For example, no null _str check in Join() member, but Clear() can set _str to null.
Probably, you never read about const member specifier, etc...

 

[Skute]

Sorry, im not understanding exactly what you mean?

Ive uploaded the full code to:

http://www.mark-ingram.com/string.zip

so you can take a look



Skute

&quot;There are 10 types of people in this World, those that understand binary, and those that don't!&quot;

 

[PerFnurt]

You don't know what a copy constructor is?
Read the copy constructor section in

http://www.cplusplus.com/doc/tutorial/tut4-1.html

/Per

_{&quot;It was a work of art, flawless, sublime. A triumph equaled only by its monumental failure.&quot;}

 

[Skute]

OK, so the copy constructor pretty much makes an exact duplicate yes?

What about a char* member variable? Would you point it at the data in the copy parameter, or would you copy the data?

ie,

either

m_pVar = Copy.m_pVar

or

memcpy(m_pVar, Copy.m_pVar)

?


Skute

&quot;There are 10 types of people in this World, those that understand binary, and those that don't!&quot;

 

[PerFnurt]

>Would you point it at the data in the copy parameter,

Well, this is what the default copy constructor does (as ArkM has pointed out) which is a root to your problems.

>or would you copy the data?

Bingo!


/Per

_{&quot;It was a work of art, flawless, sublime. A triumph equaled only by its monumental failure.&quot;}

 

[Skute]

Hmm, ive made those changes (ie the copy constructor copies the actual data) and the app crashes during a call to malloc. App flow is as follows:

String strTest = &quot;Testing this mofo&quot;;

String::String()
{
_str = NULL;
Copy(&quot;&quot;
}

String::Copy(...)
iMemSize = sizeof(char) * (int)strlen(Source) + 1;
// iMemSize = 1 here
_str = (char *)malloc(iMemSize); // Crashes here




Skute

&quot;There are 10 types of people in this World, those that understand binary, and those that don't!&quot;