malloc and free in a class

[pradeepkrao]

Hi,

I am trying to make few class similar to VB.

I thought that I will start with making String Class.

cString::~cString()
{
        free(strStr);
}
void cString::operator =(char *strNew)
{
        if (strStr != NULL)	
        {
            free(strStr);
            strStr = (char *)malloc(sizeof(char)* strlen(strNew));
        }
        else
        {
            strStr = (char *)malloc(sizeof(char)* strlen(strNew));
        }
            strcpy((char *)strStr,strNew);
}

cString &cString::operator+=(cString strNew)
{
            char * strTmp;
            strTmp = (char *) malloc(sizeof(char) * this->Len());
            
            strcpy(strTmp,(char *)strStr);

            strStr = (char *)malloc(sizeof(char)*strNew.Len());
            
            strcpy((char *)strStr,strTmp);
            strcat((char *)strStr,strNew.Value());
            free(strTmp);
            return *this;
}

class cString
{
private:
	void *strStr;
	
public:
	cString()
	{
		strStr = NULL;
	}
	
	~cString();
	
	long Len()
	{
		return strlen((char *)strStr);
	}
	
	char *Value()
	{
		return (char *)strStr;
	};

	cString &operator+=(cString strNew);
	
	void operator =(char *strNew);

	
};

I am unable to free the maemory allocated by malloc.
I am able to realloc but not free.. How am I supposed to do it..

Thanks,
Pradeep

[CornedBee]

Why don't you use new[]/delete[]?

Wait a moment, why don't you use std::string?


Besides, you're not allocating enough memory, you forget the requirement of the \0 character.

Then you can make += far more efficient:

cString &cString::operator+=(const cString &strNew)
{
    char * strTmp = new char[Len() + strNew.Len() + 1];
    strcpy(strTmp, strStr);
    strcat(strTmp, strNew.Value());
    delete[] strStr;
    strStr = strTmp;

    return *this;
}

Of course this assumes you adopt other changes, like:

const methods (especially Value() should return a const char *)

Why on earth is strStr a void*???????

The = operator should return a cString & too.

Final deallocation is done in the destructor.

You need a copy constructor and an assignment operator.



Alll in all, unless you're doing this for educational purposes, I highly recommend you use std::string.

[pradeepkrao]

Hi,

I am new to C++, though i have worked on C..

I made it void so that i can use this later for other objects..
Any way i will change it to char *.

What is the difference between

const cString &strNew and cString * strNew.

The first one is a address of a cString which cannot be modified.

and the later one is a Pointer to a cString whose data can be modifed. is that right..

I will try using your method...

I am trying to learn C++. I call it self amusement and u as educational..

Thanks,
Pradeep

[pradeepkrao]

Hi Cornedbee,


I dont know what is wrong.. but it is failing to delete[] strStr;

I copied your code and modifed the definition and still it fails..

I have tried increasing the mem etc etc.. no good..

Help me out..

Thanks,
Pradeep

[CornedBee]

You can only delete[] something that was allocated with new[].

I made it void so that i can use this later for other objects..

Typical C remnant. In C++ you have templates and polymorphism, so you usually don't need void*.
How would you use a string of objects anyway?

[pradeepkrao]

Hae Thankx Cornedbee I have now made a class..

# include<string.h>
# include<stdarg.h>
# include "cString.h"

cString::cString()
{
	strStr = NULL;
}

cString::cString(char * strNew)
{
	strStr = new char[sizeof(char)* strlen(strNew) + 1];
	
	strcpy(strStr,strNew);
}

cString::~cString()
{
	delete[] strStr;
}

void cString::operator =(cString &strNew)
{
	if (strStr != NULL)	
	{
		delete[] strStr;
		
		strStr = new char[sizeof(char) * strNew.Len() + 1];
		
	}
	else
	{
		strStr = new char[sizeof(char) * strNew.Len() + 1];
	}
	strcpy(strStr,strNew.Value());

}

void cString::operator =(char *strNew)
{
	if (strStr != NULL)	
	{
		delete[] strStr;
		strStr = new char[sizeof(char)* strlen(strNew) + 1];
	}
	else
	{
		strStr = new char[sizeof(char)* strlen(strNew) + 1];
	}
	strcpy(strStr,strNew);
}

cString &cString::operator+(cString &strNew)
{
	cString *strTmp;
	strTmp = new cString();
	*strTmp = *this;
	*strTmp += strNew;
	
	return *strTmp;
}

cString &cString::operator+(char *strNew)
{
	cString *strTmp;
	strTmp = new cString();
	*strTmp = *this;
	*strTmp += strNew;
	
	return *strTmp;
}
cString &cString::operator+=(cString &strNew)
{
	char * strTmp = new char[(Len() + strNew.Len() + 1) * sizeof(char)];	
	
	strcpy(strTmp, strStr);
	strcat(strTmp, strNew.Value());

	delete[] strStr;
    strStr = strTmp;

    return *this;
}

cString &cString::operator +=(char *strNew)
{
	char * strTmp = new char[(Len() + strlen(strNew) + 1) * sizeof(char)];	

	strcpy(strTmp, strStr);
	strcat(strTmp, strNew);

	delete[] strStr;
	strStr = strTmp;
    return *this;
}

char* cString::Mid(int startPos,...)
{
	char *strTmp;
	int length;

    va_list marker;

    va_start( marker, startPos );

    length = va_arg( marker, int);
		
    va_end( marker );

	if (length == 0 )
		length = Len() - startPos;

	if (length > 0 )
	{
		strTmp = new char[length + 1];
		memset(strTmp,0,sizeof(strTmp));
		if ((length + startPos) <= Len())
		{
			strncpy(strTmp,&strStr[startPos],length);
			strTmp[length] = 0;
		}

		return strTmp;
	}
	
	return "";
}

int cString::InStr(int startPos, char * subStr)
{
	char * tmp = strstr(&strStr[startPos],subStr);

	if (tmp != NULL)
		return tmp - &strStr[startPos] + startPos;
	return -1;
}


char * cString::LCase()
{
	return strlwr(strStr);
}
char * cString::UCase()
{
	return strupr(strStr);
}

cString.h

# include<string.h>

class cString
{
private:
	char *strStr;
	
public:
	cString();
	cString(char *strStr);
	
	~cString();
	
	long Len()
	{
		return strlen((char *)strStr);
	}
	
	char *Value()
	{
		return (char *)strStr;
	};

	void operator =(cString &strNew);
	void operator =(char *strNew);

	cString &operator+(cString &strNew);
	cString &operator+(char *strNew);
	cString &operator+=(cString &strNew);
	cString &operator+=(char *strNew);

	char* Mid(int startPos,...);
	int InStr(int startPos, char * subStr);
	char * LCase();
	char * UCase();
};

Thanks,
Pradeep

[CornedBee]

The cool thing about new is that you don't need sizeof anymore. This means that new char[23] allocates 23 chars, no matter how large a char is (it is always 1 large, as defined by the C standard).

This means that if you use sizeof for something else (like wchar_t) it will fail, because new wchar_t[sizeof(wchar_t) * len] will allocate twice the amount of memory you need.

Ok, let's look at the errors of your code. Additions by me are in bold.
I also drew the header to the front.

// cstring.h
// This name is poorly chosen, as one of the C++ standard headers is called
// <cstring>.

// Patch against VC++6 bug
namespace std {}
// C string function header, C++ style
#include <cstring>
// get the elements - I'd love to avoid this, but named bug prevents it
using namespace std;

class cString
{
private:
	char *strStr;
	
public:
	cString();
	// There's no reason why the passed string must be non-const. Parameter type changed to const.
	cString(const char *strStr);
	// You still need a copy constructor
	cString(const cString &o);
	
	~cString();
	
	// You should follow the lead that strlen sets and return a size_t
	// And I made the function const to indicate that it doesn't change the string.
	size_t Len() const
	{
		// Removed unnecessry cast.
		return strlen(strStr);
	}

	// NEVER let the user of your class modify the string internals.
	// Return type and function made const.
	const char *Value() const
	{
		// Removed unnecessary cast
		return strStr;
	}

	// Again, no need to be non-const.
	// And for the sake of chaining (a = b = c = d = "bla"), return type should be a const reference.
	const cString & operator =(const cString &strNew);
	const cString & operator =(const char *strNew);

	// All operator +s removed. They aren't usually defined in the class, but outside.
	// You can find the reason where they are defined.

	// non-const?
	const cString &operator+=(const cString &strNew);
	const cString &operator+=(const char *strNew);

	// Why variable arguments? C++ has two better mechanisms: default arguments and
	// function overloading.
	// Again, offsets should be size_t, not int.
	// And the function should please return a cString, that's the point of the class, right?
	// And it doesn't change the string, so let's make it const.
	cString Mid(size_t startPos, size_t length) const;
	// Same here, plus non-const.
	size_t InStr(size_t startPos, const char * subStr) const;
	// Return type
	cString LCase() const;
	cString UCase() const;
};

// You must not return references to unstable objects, nor must you create memory leaks.
// Return type changed to plain return-by-copy.
// Again no reason to be non-const.
cString operator +(const cString &s1, const cString &s2);
cString operator +(const cString &s, const char *sc);
// You'll undoubtly want to place the char * in front too.
cString operator +(const char *sc, const cString &s);


// cstring.cpp
// <cstring> already included via "cString.h"
// <cstdarg> not needed, we don't use it.
# include "cString.h"

cString::cString()
{
	// For the sake of the Value method and other things, it is never wise
	// to have a NULL-pointer here. Let's just initialize it to an empty string.
	strStr = new char[1];
	*strStr = '\0';
}

cString::cString(const char * strNew)
{
	// Mentioned issue
	strStr = new char[strlen(strNew) + 1];
	
	strcpy(strStr,strNew);
}

cString::~cString()
{
	// Since we can almost guarantee that strStr is not NULL, this can go unchecked.
	// On modern compilers, new should throw an exception if allocation fails.
	// On older it does not, so you should add checking after every allocation.
	delete[] strStr;
}

const cString & cString::operator =(const cString &strNew)
{
	strStr = new char[strNew.Len() + 1];
	strcpy(strStr,strNew.Value());
	return *this;
}

const cString & cString::operator =(const char *strNew)
{
	strStr = new char[strlen(strNew) + 1];
	strcpy(strStr,strNew);
	return *this;
}

cString operator+(const cString &s1, const cString &s2)
{
	// You have a huge memory leak here!
	cString strTmp(s1);
	strTmp += s2;
	return strTmp;
}

cString operator+(const cString &s, const char *sc)
{
	// You have a huge memory leak here!
	cString strTmp(s);
	strTmp += sc;
	return strTmp;
}

cString operator+(const char *sc, const cString &s)
{
	// You have a huge memory leak here!
	cString strTmp(s);
	strTmp += sc;
	return strTmp;
}

const cString &cString::operator+=(const cString &strNew)
{
	char * strTmp = new char[Len() + strNew.Len() + 1];	
	
	strcpy(strTmp, strStr);
	strcat(strTmp, strNew.Value());

	delete[] strStr;
    strStr = strTmp;

    return *this;
}

const cString &cString::operator +=(const char *strNew)
{
	char * strTmp = new char[Len() + strlen(strNew) + 1];	

	strcpy(strTmp, strStr);
	strcat(strTmp, strNew);

	delete[] strStr;
	strStr = strTmp;
    return *this;
}

cString cString::Mid(size_t startPos, size_t length) const
{
	// Must not go past end of string
	if (length <= 0 || length + startPos > Len())
		length = Len() - startPos;

	char *strTmp = new char[length + 1];
	// Return value
	cString ret;

	// Made the <= a <. Same as Len() would be already too much.[/b]
	if (startPos < Len())
	{
		strncpy(strTmp,&strStr[startPos],length);
		// Terminator handled by strncpy
		ret = strTemp;
	}
	// Avoid memory leak
	delete[] strTemp;

	return ret;
}

size_t cString::InStr(size_t startPos, const char * subStr)
{
	char * tmp = strstr(&strStr[startPos],subStr);

	if (tmp != NULL)
		// Why so complicated?
		return tmp - strStr;
	return -1;
}

// Rewrite those two. strlwr and strupr are not standard methods.
// Use tolower and toupper from <cctype> for it. Loop through the strings.
// Make sure you don't have memory leaks.
cString cString::LCase()
{
}
cString cString::UCase()
{
}

[pradeepkrao]

Hi Cornedbee,

Thanks a million for that detailed explanation, which I cud hardly find anywhere.

I Have couple of doubts in C++.

1) when I want to overload the operator should I have to use

cString &operator+(cString &strNew);
or
cString operator+(const cString &s1, const cString &s2);

What is the difference between the above?.
I cud interpret the first one as

(this + strNew)

Where as the second one I cud'nt figure out
Looks like s1+s2. Is that right..

In this code i have only one argument which is passed.

cString &cString::operator+=(cString &strNew)
{
char * strTmp = new char[(Len() + strNew.Len() + 1)];	
	
strcpy(strTmp, strStr);
strcat(strTmp, strNew.Value());

delete[] strStr;
strStr = strTmp;

return *this;
}

Thank you once again.

Pradeep

[pradeepkrao]

Originally posted by CornedBee

cString operator+(const cString &s1, const cString &s2)
{
	// You have a huge memory leak here!
	cString strTmp(s1);
	strTmp += s2;
	return strTmp;
}

But CornedBee ... strTmp is a Local variable and will its destructor not be called when it goes out of scope. If so how there cud be a memory leak.

Exactly where does it loose the scope...?... After function exit's..?..

//I am returning the position where the substring is found.
//Similar to InStr in VB

int cString::InStr(int startPos, char * subStr)
{
	char * tmp = strstr(&strStr[startPos],subStr);

	if (tmp != NULL)
		return tmp - &strStr[startPos] + startPos;
	return -1;
}

Thanks,
Pradeep

[CornedBee]

1) when I want to overload the operator should I have to use

The difference is that the first is defined in the class, the other is a global function. For addition of two class objects it doesn't matter, but for a class object and a primitive (like const char *) it does, because the first version (in the class) always needs the class as first argument (the hidden this).
This means you can write
cString str("foo");
str = str + "bar";
but not
str = "bar" + str;

The global functions support the second line too. This is why you always define them outside the class and implement them via the += operator. And consequently, to make the operators easier to find, you define all + operators in a group, i.e. outside.

strTmp is a Local variable and will its destructor not be called when it goes out of scope.

Sorry, should have said "you had a memory leak". I corrected it.
Let's look at your code again.

	cString *strTmp;
	strTmp = new cString();
	*strTmp = *this;
	*strTmp += strNew;
	
	return *strTmp;

The problem is that you allocate the cString on the heap. This means that the pointer is local, yes, but the cString is on the heap. It doesn't go out of scope when you leave the function, only the pointer does. This means that the object is left on the heap with no means of recovering the memory: a leak.

return tmp - &strStr[startPos] + startPos

Why did I change this code?
Let's see. &strStr[startPos] is the same as &(*(strStr + startPos)). The & and the * cancel each other out, leaving us with (strStr + startPos).
So the whole line is
tmp - (strStr + startPos) + startPos
= tmp - strStr - startPos + startPos
= tmp - strStr + (-startPos + startPos)
= tmp - strStr

You see, a little understanding of what the [] operator does leads to shortening of the code.

[pradeepkrao]

Hae Thankx CornedBee.

Yeah I know that my previous code had a memory leak as i was allocating the memory and not deallocating.. Cuz i dint know where to deallocate..

I used the one what you told before.. but i had a problem.. when i said a = b + c

'b + c' returned the object and the memory refference was assigned to 'a'... But when the memory allocated in the function went outof scope 'a' had become a Invalid Pointer.

I will try out your methods and then learn to optimize..

I am so used to VB that I dont think of code optimizing.. ..

Thank you once again..

Where can i find few good articles to learn C++..

Thanks,
Pradeep

[CornedBee]

Your class should keep track of two things: the string length (so that you don't have to calculate it every time) and the amount of allocated memory. This is a huge opportunity for optimization. Your current class is very inefficient because it ALWAYS reallocates the memory. An efficient string class keeps track of how much memory it has and only reallocates if it needs more memory. This means for example when you assign a small string to an object that previously held a large string, you don't need to reallocate.
Then you need an allocation strategy for building strings. For example, if a user keeps adding a single character to a string (like in a loop) and you have a "allocate-only-what-you-need" strategy, you have to allocate every loop iteration. The most common strategy is "double-when-out": whenever you run out of memory, allocate twice as much as currently held.

The MS STL implementation of std::string also contains an automatically allocated area for 16 characters in order to store small strings quickly. However, this approach makes it complicated.

[pradeepkrao]

Cool.. Ok.. in that case I will be having few unused memory block.
That is not a big deal cuz this is 2003 and the RAM's are cheap.
Yes we need to concentrate on performance..

Hae thankx..

Please suggest me few good articles..
I am not new to C.. but yes to C++.. Though i have coded in C++, I wrote them all in C style...

Thanks,
Pradeep

[CornedBee]

No idea about articles. I'm writing a C++ tutorial myself which will eventually find its way to
http://stud3.tuwien.ac.at/~e0226430/
but I'm only at functions yet, with little time and it's currently only available on my local server.