function return temporary object misbehaving

Question

OK, very simple String class, which holds constant strings (i.e. can't be changed once initialized), implements the copy ctor and concatenation function conc. It's that function that's giving me trouble, because I really cannot figure out why a local variable I make isn't passed normally as return value.

Code:

class String
{
public:
    String(char*);
    String(const String& other);
    ~String();
    friend String conc(const String&, const String&);
    friend std::ostream& operator<<(std::ostream&, const String&);
private:
    const char* const str;
};

String::String(char* aStr) : str(aStr) {}

String::String(const String& other) : str(other.str) {}

std::ostream& operator<<(std::ostream& out, const String& s)
{
    out<<s.str<<"\n";
    return out;
}

String::~String()
{
    delete str;
}

String conc(const String& s1, const String& s2)
{
    int n = strlen(s1.str) + strlen(s2.str);
    int i, j;
    char* newS = new char[n+1];
    for(i = 0; i < strlen(s1.str); ++i)
    {
        newS[i] = s1.str[i];
    }
    for(j = 0; j < strlen(s2.str); ++j)
    {
        newS[i+j] = s2.str[j];
    }
    newS[i+j] = '\0';  //newS is made correctly 
    String tmp(newS); // the tmp object is made correctly
    return tmp;   // here tmp becomes something else --- Why??
}

int main()
{
    String s1("first");
    String s2("SECOND");

    String s3 = conc(s1, s2); //here the copy ctor should be called, right?
    std::cout<<s3;
    _getch();
}

As you can see in the comments, the problem is in the conc function at the end. I have made the function return a String, and not String& on purpose, given that the value it returns shouldn't be an lvalue...

Please explain & help, thanks!:)

Your copy constructor fails to do a "deep" copy; it simply copies a pointer to the memory that your `tmp` object deletes (invalidly) in its destructor. — eq-, Sep 11 '12 at 16:50
@eq- aaaaaaaah, so in the line `String s3 = conc(s1, s2);` the copy ctor assigns the char array which is then deleted by tmp upon destruction? — Vidak, Sep 11 '12 at 16:52
possible duplicate of [What is The Rule of Three?](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) — Mike Seymour, Sep 11 '12 at 16:54

score 6 · Accepted Answer · answered Sep 11 '12 at 16:56

6

There are memory management problems all over the place here. Each String object should own its own memory. That means that String::String(char*) needs to allocate an array of char and copy the contents of the input string; String::String(const String&) needs to allocate an array of char and copy the contents of the input string; and String::operator= needs to delete its own string (unless it's the same as the input string), allocate an array of char, and copy the contents of the input string. Finally, String::~String() should delete[] the array of char.

answered Sep 11 '12 at 16:56

Pete Becker

74,985
8
76
165

No doubt that there are memory management problems in this code, but you're incorrect to say that each object should make a copy. That's the beauty of an immutable string type, they can all share the same data. What you need is a smart-pointer like arrangement where the last object destroyed also destroys the data. – Mark Ransom Sep 11 '12 at 18:07
@MarkRansom - that's another way of implementing it. Feel free to write it up as an answer. – Pete Becker Sep 11 '12 at 18:10

score 4 · Answer 2 · answered Sep 11 '12 at 16:51

4

It is not what you think: the problem is the deletion of the temp. You call delete instead of delete[] on an array allocated with new[] - an undefined behavior.

Once you fix that error, you will have other errors related to Strings initialized with string literals: passing them to delete[] is also undefined behavior.

The root cause of the problem is that your class does not let you differentiate between the memory that you must release and the memory that you must not release. You should do it uniformly, for example by always copying the content into an array that you allocate inside the constructor.

answered Sep 11 '12 at 16:51

Sergey Kalinichenko

714,442
84
1,110
1,523

@eq- I was in the process of incrementally building my answer :) – Sergey Kalinichenko Sep 11 '12 at 16:52
Ok, but how do I then initialize my `str` in the constructor? Suppose I do create a new array inside the constructor, how do I assign it to `str`? – Vidak Sep 11 '12 at 17:02
1

@Vidak For example, like this: `str(new char[strlen(aStr)]) {strcpy(str,aStr);}` – Sergey Kalinichenko Sep 11 '12 at 17:04
wow cool!! is that the only way to initialize a `const` variable? To use it with that `: variable(//...) {}` construction? – Vidak Sep 11 '12 at 17:08
1

@Vidak Yes, `const` variables need to be initialized in the init list. – Sergey Kalinichenko Sep 11 '12 at 17:09

score 2 · Answer 3 · answered Sep 11 '12 at 16:57

There are several problems with your class:

Your String( char * ) constructor assumes ownership of the pointer passed to it, so if you construct the object with a string literal the destructor will try to delete it, leading to undefined behavior.
Your copy constructor assumes ownership of the string belonging to the object being copied, instead of making its own copy, due to which the string will be double deleted.
In the conc function you allocate memory using new[] but then you delete it instead of delete[] leading to undefined behavior
The member variable str is supposed to be a char array, so the destructor must delete[] it instead of delete

How can I create a new copy in the copy ctor? `str` is `const`, I can't just assign a value to it :( — Vidak, Sep 11 '12 at 17:04
Why is `str` `const`? It's a private member variable, so there's no reason to impose additional restrictions of constness on it. — Praetorian, Sep 11 '12 at 17:25

piokuc · Answer 4 · 2012-09-11T16:57:51.223

You need to change your constructor so it's making a copy of the C string, for example:

String::String(const String& other) : str(strdup(other.str)) {}

If you use strdup above you should change your destructor appropriately, so instead of using delete you use free

String::~String() { free(str); }

It would be a good idea to change your other constructor so it's not acquiring the C string, but making a copy of it as well, this way behaviour of all constructors would be more consistent and safer, in general:

String::String(const char* aStr) : str(strdup(aStr)) {}

If you make it this way, it will work correctly whether the client code is passing you pointer allocated with malloc or new.

Replacing strdup and free with more new and delete should be easy, I'm leaving it to you as an exercise.

You can't use `strdup` (although there isn't a formal standard for it) with `delete` or `delete[]`. — eq-, Sep 11 '12 at 16:51

score 1 · Answer 5 · edited May 23 '17 at 12:03

When you return a temporary from a function, a copy is made for the return value and the temporary is destroyed. (Sometimes the copy can be skipped via return value optimization but I don't think that's happening here).

Because your copy constructor copies the character array pointer, both objects are now pointing to the same memory. When the temporary is destroyed, it destroys the character array and now the returned object has a dangling pointer.

One of the big benefits of an immutable string class is that it can easily share buffers as you do here, without the overhead of copying. You just need a mechanism for counting the references to the buffer so it isn't deleted until the last object is deleted. You can use a std::shared_ptr with a custom deleter instead of the char * pointer.

You should also look into the Rule of Three to make sure you're implementing all the necessary functions.

function return temporary object misbehaving

5 Answers5