improving my string implementation

String::operator+=() leaks memory.

Thanks, I fixed that I think.

Nope.

giblit (3750)

@ OP if you use new [] you must use delete [] and if you use new you must use delete. So in the case of your += overload, you use new [] and then use delete which causes the leak.

I would have preferred to let OP figure it out.
Anyway, there's another leak besides what giblit said.

giblit (3750)

I just mentioned that because sometimes something as simple as that you may just overlook without thinking twice. Anyways, here is the article to dynamic memory. http://www.cplusplus.com/doc/tutorial/dynamic/

Thanks giblit. And helios, I don't think I see it.

I know I create a few arrays in there. Like where my String::operator=() is. But I thought the destructor took care of that one.

Isn't the L-value of that buffer attached to the string which is being set to equal the one being passed in the argument?

Last edited on

But I thought the destructor took care of that one.

Have you considered carefully what happens when you assign a new value to an existing string?

1
2
3

String test("Hello");

test = "Hello leak!";

and you should also think about

1
2
3

String test("Hello");

test = test;

Andy

PS As the code stands there are two leaks.

Last edited on

When you do this with my current code:

1
2
3

String test("Hello");

test = "Hello leak!";

Does it create a memory leak because it never releases the memory that holds "hello" and allocates new memory for "Hello leak"?

Last edited on

Does it create a memory leak because it never releases the memory that holds "hello" and allocates new memory for "Hello leak"?

Yep!

Andy

Last edited on

Awesome! Now I've updated my code but I am curious if I still have leaks here. I believe I fixed it but I may have overlooked something.

#include <iostream>
#include <cassert>

using namespace std;

class String {
        int size;
        char * buffer;
public:
        String();
        String(const String &);
        String(const char *);
        ~String();
        int length() const;
        char& operator[] (unsigned int);
        void operator =(const String&);
        void operator += (const String&);

        	// other methods
        friend bool operator==(const String &, const String &);
        friend bool operator<=(const String &, const String &);
        friend bool operator<(const String &, const String &);
        friend ostream & operator<<(ostream &, const String &);
};

String::String()
{
    buffer = nullptr;
    size = 0;
}

String::String(const String&s)
{
    size = s.size;
    buffer = new char[size];
    for(int i = 0; i < size; i++){
        buffer[i] = s.buffer[i];
    }
}

String::String(const char *p)
{
    int i = 0;
    const char * t = p;

    while(*p++)
    {
        i++;
    }

    buffer = new char[i];
    int j = 0;

    for(j;*t;t++,j++)
    {
        buffer[j] = *t;
    }
    size = j;
}

String::~String()
{
    delete[] buffer;
}

int String::length() const
{
    if(buffer == nullptr)
    {
        return 0;
    }
    else
    {
        return size;
    }
}

char & String::operator[] (unsigned int x)
{
    return buffer[x];
}

void String::operator =(const String&s)
{
    delete [] buffer;
    size = s.size;
    buffer = new char[size];
    for(int i = 0; i < s.length();i++)
    {
        buffer[i] = s.buffer[i];
    }
}

ostream & operator<<(ostream &os, const String &s)
{
    for(int i = 0; i < s.size; i++)
    {
        os << s.buffer[i];
    }
    return os;

}

bool operator==(const String & s, const String & t)
{
    if(s.length() != t.length())
    {
        return false;
    }
    else
    {
        for(int i = 0; i < s.length(); i++)
        {
            if(s.buffer[i] != t.buffer[i])
            {
                return false;
            }
        }
    }
    return true;

}

void String::operator += (const String& s)
{
    char * temp = new char[size];
    int t = length();

    for(int i = 0; i < length(); i++)
    {
        temp[i] = buffer[i];
    }

    int tsize = size + s.size;
    buffer = new char[tsize];

    for(int i = 0; i < length(); i++)
    {
        buffer[i] = temp[i];
    }

    for(int i = t, j = 0; j < s.length(); i++,j++)
    {
       buffer[i] = s.buffer[j];
    }
    size+=s.size;
    delete [] temp;
}


int main()
{
   String s1; // s1 == ""
   assert(s1.length() == 0);

   String s2("hi");  // s2 == "hi"
   assert(s2.length() == 2);

   String s3(s2);  // s3 == "hi"
   assert(s3.length() == 2);

   assert(s3[0] == 'h');
   assert(s3[1] == 'i');

   s1 = s2;  // s1 == "hi"
   assert(s1 == s2);

   s3 = "bye";  // s3 == "bye"
   assert(s3.length() == 3);
   assert(s3[0] == 'b');
   assert(s3[1] == 'y');
   assert(s3[2] == 'e');

   s1 += "re";  // s1 == "hire"
   assert(s1 == "hire");

   s1 += "d"; // s1 == "hired"
   assert(not (s1 == "hire"));

   cout << "SUCCESS" << endl;
}

Last edited on

You still have one leak.

And you are not handling

1
2
3

String test("Hello");

test = test;

Andy

Yes, there's still one other leak.
Also, I just noticed possible illegal accesses in String::String(const String&) and String::operator=(const String&), and two more in String::operator+=().

You're missing test cases:

1. copy constructor from empty object (one which is default constructed)

2. operator= with empty object

3. operator= with same object

4. operator+= with empty object

5. operator+= with same object

And more!

(It might help to split out test cases into separate functions?)

Andy

Last edited on

I'm not familiar with illegal accesses.

And thanks for the additional test cases Andy. I am seeing my implementation fail as I add some of those in.

#3 seems to be causing a crash as it is.

I'm not familiar with illegal accesses.

This is what prompted me to suggest the additional test cases.

For example...

String::String(const String&s)
{
    size = s.size;
    buffer = new char[size];
    for(int i = 0; i < size; i++){
        buffer[i] = s.buffer[i]; // what memory are you accessing here if s is empty??
    }
}

#3 seems to be causing a crash as it is.

Hopefully this should become clear if you think about what's happening to the buffer?

Andy

Last edited on

So I believe I have to make a check to see if the buffer is empty? For the illegal accesses.

And how would I handle this? It fixes the problem but I'm not sure if I'm creating another problem by doing this.

void String::operator =(const String&s)
{
    if(buffer == s.buffer)
    {
        //do nothing
    }
    else{
        delete [] buffer;
        size = s.size;
        buffer = new char[size];
        for(int i = 0; i < s.length();i++)
        {
            buffer[i] = s.buffer[i];
        }
    }
}

By the way thanks for the great feedback and help. I feel like I am learning a lot about memory management.

Last edited on