Should I return reference to heap object or return value?

Question

I have these two simple functions. I thought that func1 was a good solution since you pass an object by reference. My textbook gave func2 as the answer for the best solution. Is this only because you aren't deallocateing heapstr? What if I declared heapstr in main and then passed it to the function so I was able to delete it afterwards?

#include <iostream>

using namespace std;


string& func1(const string &str) {
    string* heapstr=new string();
    for (int i = 0; i < str.size(); ++i) {
        *heapstr += str[i];
    }
    return *heapstr;
}

string func2(const string &str) {
    string heapstr;
    for (int i = 0; i < str.size(); ++i) {
        heapstr += str[i];
    }
    return heapstr;
}

int main() {
    cout << func1("aaa") << endl;
    cout << func2("aaa") << endl;
}

Answer 1

Should I return reference to heap object or return value?

Return by value.

There are a lot of reasons why, but none are really related to performance, because the compiler is good enough at optimising things, and even if it wasn't, most programs are I/O-bound, i.e. the time you wait for data from files or network sockets eats up all your performance, not the time spent by CPU operations themselves.

See for example the "C++ Core Guidelines" by Herb Sutter and Bjarne Stroustrup, which say at section "Return containers by value (relying on move or copy elision for efficiency)":

Reason

To simplify code and eliminate a need for explicit memory management.

As for your two functions...

My textbook gave func2 as the answer for the best solution. Is this only because you aren't deallocateing heapstr?

The memory leak is one of the problems. But the point is simply that returning by value is simpler and less error-prone. It's all about correctness, not speed. You wouldn't return an int* if you could just return an int instead, would you?

What if I declared heapstr in main and then passed it to the function so I was able to delete it afterwards?

You would introduce a lot of possibilities for memory leaks, crashes and undefined behaviour into your code. It would become longer, harder to write, harder to read, harder to maintain, harder to debug and harder to justify in a code review. In return, you would gain absolutely nothing.

Answer 2

The text book is correct. (Shocker.)

Func1 is faulty in every respect in which it differs from func2. It allocates an object from the heap without regard to how that object will be deleted. Then it returns a reference to the new object, hiding the pointer that might have otherwise been used to delete it. There is no efficiency gain, in fact Func1 is probably a bit slower. In any case, recite after me: "Avoid Early Optimization."

Since the advent of the Standard Template Library, many moons ago, it is almost never best to use operator new. The last time I used operator new was ca. 2003, and I wrapped the pointer in the equivalent of what we now know as a unique_ptr. Before you use operator new, read all about learn all about smart pointers and RAII.

Answer 3

Since this is textbook example, you should consider its context to underestand what it wants to show exactly(its goal is to minimize memory usage as you or using a safe programming pattern?!). But two hint

1. When you use new operator to allocate memory, you must de-allocate it using delete. The code has memory leak for heapstr at func1.
2. Also in the more realistic projects its not safe to share objects between methods. Its management(i.e who currently modified this or who is responsible to de-allocate its memory when object is not needed longer) became hard.

PS: I do not have C++17 but it optimize following as well. For more details read @BoPersson comments.

PS: Stack allocation is faster, but in your example you have a copy operation at func2 return. In your example as @Jive Dadson said there is no difference due ot compiler optimization but in general case, suppose following code

#include <iostream>
#include <string>
using namespace std;


string& func1(const string &str) {
    string* heapstr = new string();
    cout << "func1 " << heapstr << endl;
    for (int i = 0; i < str.size(); ++i) {
        *heapstr += str[i];
    }
    return *heapstr;
}

string func2(const string &str) {
    string heapstr;
    for (int i = 0; i < str.size(); ++i) {
        heapstr += str[i];
    }
    cout << &heapstr << endl;
    return heapstr;
}

int main() {
    string a = func1("aaa");
    string b = func2("aaa");
    cout << "main " << a << endl;
}

PS: (As @Jive Dadson said there is no difference in your example, but in mine)If we define performance as run time, maybe func1. Also if we define performance as memory usage, func1. If we define performance as good programming pattern, func2. Totally func2 is more preferred.