What is wrong with this simple C++ function?

Question

Here is a question from an interiew, and the code attached below. What is wrong with this function?

string f() {
    return "hello world";
}

To me, there is nothing wrong at all, and I can even run a program using this function:

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

string f() {
    return "hello world";
}

int main() {
    string s2=f();
    cout<<s2<<endl;
}

What is wrong with this function?

Answer 1

Well... have some consolation, that in this type of question, there's often no right answer. The best you might hope is to wow the interviewers by showing your expertise to elaborate the pros and cons, (a.k.a. code masturbation).

From a style point of view, its often bad to return objects using return-by-value. Consider:

X f();

X x;
x = f();

f allocates an X. An X needs to be returned, so an additional copy is placed on the stack as the return value. Finally, the x on the stack is copied into x via the assignment operator. In total, 3 X's appear in memory. Some might say this is inefficient, and so you should try to not return-by-value, when the value is an object.

However, a more savvy interviewee might have pointed out that:

• some compilers optimize out the temporary copy .For this optimization, I believe it makes a difference here whether the assignment to X is made via the copy constructor or equality operator. Say "copy elison" and see if the interviewer raise and eyebrow: What is copy elision and how does it optimize the copy-and-swap idiom? . I'm guessing copy elision might work here because the function is in the same translation unit, and therefore easily inlineable.
• almost all implementations of the string class implement copy on write, in which a single copy of the string's text data would be held in this case, with the rest of the data being held on the stack and having basically causing no overhead when the string is copied.
• if you're working with an early version of visual studio, memory leaks will ensue

So, what alternatives?

X const& f();

may be considered as an alternative. However, this has its own style problems as the lifetime of the result is unclear to the caller. Perhaps the next call invalidates the previous reference result? Who knows?

Also

void f_get( X& result );

may be preferred (by some interviewers). This has the benefits:

• no copies of local variables
• no references/pointers of indeterminate lifetimes
• in the case where X is string, one only copy of the text data is ever held (although this is true is all cases)

In both, readability is sacrificed -- in the general cases, the caller now has to pay more attention to which arguments are functional parameters, and which are intended to hold results.

In the O.P. it not may also not be obvious what the lifetime of string literal on the stack is. Is it de-allocated from the stack when the function returns? Probably not -- its likely kept in the static memory area (best check the spec to be sure). But if not, then what's the consequence - probably none since the string probably makes a copy of char const* on the heap, unless you have some weird string implementation who's constructors can tell the distinguish literal from non-literal parameters with some clever template type-trait magic.

In any case, spouting some B.S. like this would have surely scored points, if off the mark.

Also, is your console outputting ascii, UTF-8 or unicode? The program's probably wrong for one of them, and also wrong in no English speaking locales.

Did you check that stdout has a valid value, or are you compiling without -D_CONSOLE in windows, or for some embedded device or gaming console where stdout is not available and you must redirect out to a logging library (or crash)?

Meh... take your pick.

The temporary copy was probably the droid they were looking for. Because the temporary is of no consequence in the case of a string (and probably also copy elision), the interviewer smells of elderberries and his mother was a goat

Answer 2

There's nothing wrong per se with this function - it's totally safe. Since this function always returns the same string, however, you could consider replacing the function with a constant, like this:

static const string kHelloWorld = "hello world";

cout << kHelloWorld << endl;

This has the advantage that there's only one copy of this string ever and it now is a named constant, meaning that it can be changed as necessary and there's no overhead for the function call and return.

Hope this helps!

Answer 3

string f() {
    return "hello world";
}

As per templatetypedef's answer, this will result in the construction of a string temporary at each point of use.

You can address this with:

const std::string& f()
{
    static const std::string hw("Hello world");
    return hw;
}

This way, multiple calls yield references to a single std::string instance, which is created during the first call. There may be a performance cost each time f() is called, while it checks if local statics have already been created - I'm not sure what the state of the art in compiler approaches to this is.

By way of contrast, if you have client code directly use a constant...

static const std::string hw("Hello world");

...then you have to get every client to update their code if you ever want to start returning a runtime-determined value (e.g. picking a "Hello world" message in a language based on an environment variable). If you'd stuck with the function, then you could choose which of several constants to return a reference to, or you could change the return type from by-const-reference to by-value and return any dynamically generated text.