Rewriting += Operator C++

Question

UPDATE: Allocated memory for str1's new data. Still memory error.

I'm trying to rewrite the += method for a string class I created.

Class mystring{

public:
    friend void operator+=(mystring& str1, const mystring& str2){
        mystring temp;

        delete[] temp.data;
        temp.length = str1.length + str2.length;
        temp.data = new char[temp.length + 1];

        strcpy(temp.data, str1.data);
        strcat(temp.data, str2.data);

        delete[] str1.data;
        str1.length = temp.length;

        strcpy(str1.data, temp.data);

    }

private:
    char *data;
    int length;

}

Then in the main class:

mystring str1("hi");
mystring str2("matt");

str1 += str2;
cout << str1 << endl;

This function is working as it should be, but I am getting memory errors all over when I run valgrind. I am unable to figure out why that is. If anyone could give me any tips that would be awesome.

Thanks

please put the relevant errors being shown in valgrind and also your constructor code . — Arunmu, Dec 06 '11 at 03:39
Note that you should define a swap method. You should probably have a special mystring constructor so you don't have to delete temp.data first. You should use memcpy since you know the lengths: strcpy and cat are wasteful. With the swap method after making temp the new string you'd just swap(*this, temp). Swap would just swap the pointers and the lengths. — Zan Lynx, Dec 01 '12 at 02:22
Another comment, if you want better append performance you might want to use good old realloc() instead of new and delete. OFten the allocator can find new memory at the end of a block which means it doesn't need to move the old data to a new position: it just extends the allocation block. — Zan Lynx, Dec 01 '12 at 02:24

score 2 · Answer 1 · answered Dec 06 '11 at 03:38

2

You need to allocate additional memory in str1.

You can't just blindly copy past the end of the array.

answered Dec 06 '11 at 03:38

SLaks

868,454
176
1,908
1,964

Keith · Accepted Answer · 2011-12-06T05:19:02.603

Firstly, you meant not:

 strcat(str1.data, str1.data);

but:

 strcat(str1.data, str2.data);

Secondly, where do you expect str2.data to go? It is a memory scribble and hence the valgrind errors. Surprised it does not just crash.

You need to reallocate enough storage for the combined length, copy over both original strings and free str1.data before re-assigning it to the new storage.

Based on the updated post:

friend void operator+=(mystring& str1, const mystring& str2)
    {
        // Not using a temp mystring here, as the temp never really maintains its state as a mystring
        // I am assuming length is the length of the string, not the storage. Not the best design if you consider resizing the the string to less than the storage
        int newStringLength = str1.length + str2.length;
        char* newStorage = new char[newStringLength +  1];

        strcpy(newStorage, str1.data);
        // strcat has to scan from the start of the string; we do not need to.
        strcpy(newStorage + str1.length, str2.data);

        delete[] str1.data;

        str1.length = newStringLength ;
        str1.data = newStorage;

         // Haven't though about the case where str2 is an alias for str1.
    }

Thanks for the reply. I have changed my code to try to use your method, but it is still giving me memory errors. The OP has been updated. — KWJ2104, Dec 06 '11 at 03:54
"Haven't though about the case where str2 is an alias for str1." - Why would anything be wrong if you do `s += s;`? `s` just contains the original string repeated twice now. — visitor, Dec 06 '11 at 09:44
@visitor. Hadn't thought about means its worth checking. A quick read through now suggests to me that this would be OK - but I can easily think of alternative implementations that would break for the alias case only. — Keith, Dec 07 '11 at 02:21

score 1 · Answer 3 · answered Dec 06 '11 at 03:38

1

You have to allocate heap to hold characters and release the heap when it is no longer needed.

Something like this:

data=new char[length+1];

answered Dec 06 '11 at 03:38

KV Prajapati

93,659
19
148
186

score 1 · Answer 4 · answered Dec 06 '11 at 04:02

 //it is strange that operator += return void
 // usually we have T& operator += (T const&, T const&) 
  //or T& T::operator +=(T const&)
 friend void operator+=(mystring& str1, const mystring& str2){
    //make sure str1 and str2 are correclty initialzed
    str1.length = str1.length + str2.length;
    //make sure str1.data has enough memory to hold all the data
    //make sure str1.data and str2.data are null terminated strings, not binary data
    strcat(str1.data, str2.data);
}

Rewriting += Operator C++

4 Answers4