Overloading += operator, unexpected result

Question

I wrote my own string class for learning purposes. I'm trying to overload the += operator so that I can append strings together. But whenever I use the operator the inital object remains the same. I am confused.

StringF& StringF::operator+=(StringF& obj) {
    const char* string = this->getString();
    const char* stringToAppend = obj.getString();

    const int stringLength = this->length();
    const int stringToAppendLength = obj.length();

    char* appendedString = new char[stringLength + stringToAppendLength];
    appendedString[stringLength + stringToAppendLength] = '\0';


    for (int i = 0; i < stringLength; i++) {
        appendedString[i] = string[i];
    }

    for (int i = stringLength; i < stringLength + stringToAppendLength; i++) {
        appendedString[i] = stringToAppend[i - stringLength];
    }

    StringF appendedObj = StringF::StringF(appendedString);

    std::cout << "Appended obj: " << appendedObj.getString() << std::endl; //For debugging

    return appendedObj;
}

Here is the class header file:

class StringF {
    private:
        const char* string;
    public:
        StringF(const char*);
        int length();
        const char* copy();
        const char* getString();
        const char* reverse();
        int find(const char*);
        StringF& operator+=(StringF&);
        friend std::ostream& operator<<(std::ostream&, StringF&);
};

I want to be able to create two strings and append with one another. But the it doesn't work. Any ideas? If possible, I'd want an explanation as to why this doesn't work so I can try to figure it out on my own, as I am trying to learn I don't want the solution handed to me. But I'll take whatever.

So here is what I'm trying to do.

StringF s("Hello");
StringF s2(", World!");
s += s2;
std::cout << s << std::endl; //Should print "Hello, World!" but only prints "Hello".

Thanks in advance!

Edit: Here is the example for you to try. https://onlinegdb.com/ApfkIONXG

Provide a [minimal reproducible example](https://stackoverflow.com/help/minimal-reproducible-example). You can check your code [here](https://onlinegdb.com/gx6fAwbS6L) — Jason, Jan 20 '23 at 05:57
`appendedString[stringLength + stringToAppendLength] = '\0';` - buffer overflow. — 273K, Jan 20 '23 at 06:03
`operator+=` is supposed to be modifying the `this` instance then return a reference to `this`. Your implemented is returning an entirely new instance so `s += s2` is not modifying `s`. And you have a buffer overflow as described in the other comment. — Dean Johnson, Jan 20 '23 at 06:06
Undefined behaviour due to off by one error. `char* appendedString = new char[stringLength + stringToAppendLength]` followed by `appendedString[stringLength + stringToAppendLength] = '\0'` overwrites a character that is one past the end of the allocated `appendedString`. Also, the `operator+=()` is returning a reference to a local variable, of automatic storage duration, that will cease to exist when the function returns. — Peter, Jan 20 '23 at 06:07
`StringF appendedObj = StringF::StringF(appendedString); . . . return appendedObj;` looks to me like you are returning a reference to a local objuct that goes out of scope when the function exits. That is never valid. — Avi Berger, Jan 20 '23 at 06:07
Okay thank you for pointing out the buffer overflow. Even tho I looked at the code for hours I didn't see that one. However, @DeanJohnson how would you suggest I go about this then? Because I can't modify a const char* so I thought I have to return a new object. — Darawan, Jan 20 '23 at 06:09
@AviBerger ofc. I have to allocate it on the heap if I want to do that. I just realized, man c++ is hard.... — Darawan, Jan 20 '23 at 06:10
(1) What does `valgrind` say about this violation of array bounds? `appendedString[stringLength + stringToAppendLength] = '\0';` (2) Where is `delete[] string`? (3) Why not `return *this;`, which is what `operator +=(...)` should always do? — Andrej Podzimek, Jan 20 '23 at 06:18
But as @DeanJohnson said you, you don't want a new object, you want to modify the left hand side object which is pointed to by the `this` pointer. So maybe `delete this->string; this->string = appendedString;` You will also have to worry about the rules of 3/5 (deal with destructor copy/move constructors & copy assignment.), — Avi Berger, Jan 20 '23 at 06:18
If you want to modify an object on the left hand side of an expression `a += b`, then it is usually a mistake for that object to have a `const` member. It is in this case - particularly since you are trying to change the size of the buffer AND overwrite characters pointed to by that `const` member. — Peter, Jan 20 '23 at 06:24
Anyhow, in most cases, a good start is to make the code `valgrind`-clean first, such that it doesn’t touch memory it shouldn’t be touching and doesn’t leak memory all over the place. [Here’s what the `operator +=` could look like](https://pastebin.com/9AzT4c2k), without memory leaks. — Andrej Podzimek, Jan 20 '23 at 08:01

Dean Johnson · Answer 1 · 2023-01-20T06:39:48.223

There are a few issues with the code as posted:

There is a buffer overflow
You are not modifying the this instance like operator+= is supposed to
You are returning a reference to a local

Here is some minimally edited code that fixes these issues

StringF& StringF::operator+=(StringF& obj) {
    const char* string = this->getString();
    const char* stringToAppend = obj.getString();

    const int stringLength = this->length();
    const int stringToAppendLength = obj.length();

    // +1 for the null character we are appending
    char* appendedString = new char[stringLength + stringToAppendLength + 1];
    appendedString[stringLength + stringToAppendLength] = '\0';


    for (int i = 0; i < stringLength; i++) {
        appendedString[i] = string[i];
    }

    for (int i = stringLength; i < stringLength + stringToAppendLength; i++) {
        appendedString[i] = stringToAppend[i - stringLength];
    }

    // TODO: de-allocate the previous 'string' member variable

    // Now we need to update `this`. While we can't modify the data pointed
    // at by the existing member pointer, we can change the pointer to point
    // to the newly allocated data:
    string = appendedString;

    // operator+= should be returning a reference to `this`
    return *this;
}

We can modify the member variable because const char* is a mutable pointer to constant characters. This means the characters at the address of the pointer are constant, but the memory storing the pointer address is still mutable.

|string: 0xDEADBEEF|           // The pointer value is not const
            |
            |----> |abc\0|     // This data is const

see Const before or const after? for more info on the const issue.

The return *this is because the general expectation of operator+= is that the following code should work:

StringF s1("a");
StringF s2("b");
StringF s3 = (s += s2);
assert(s3 == s);

This might seem like non-sense code in this simple example, but this pattern is useful in some cases and is the expected behavior of a operator+= overload.

Overloading += operator, unexpected result

1 Answers1