How to properly return std::string (or how to properly use that returned value)

Question

Say you have a class which is a global (e.g. available for the runtime of the app)

class MyClass {
  protected:
    std::string m_Value;
  public:
    MyClass () : m_Value("hello") {}
    std::string value() { return m_Value; }      
};

MyClass v1;

Using the first form gives me odd behavior when I do

printf("value: %s\n", v1.value().c_str());

It looks as though the string disappears from memory before printf can use it. Sometimes it prints value: hello other times it crashes or prints nothing.

If I first copy the string like so

   std::string copiedString = v1.value();
   printf("value: %s\n", copiedString.c_str());

things do work.

Surely there must be a way to avoid doing this with a temporary string.

Edit: So the consensus is to go with a const std::string & return value.

I know everyone says that the original code should be fine but I can tell you that I've seen MSVC 2005 on Windows CE having trouble with it, but only on the CE box. Not the Win32 cross compile.

Answer 1

Your code should work fine. Something else is wrong, that we can't detect from this testcase. Perhaps run your executable through valgrind to search for memory errors.

Answer 2

Well, there's nothing wrong with the code (as I interpret it). It is not optimal and surely not The Right Way (R), you should modify your code like villentehaspam suggests. As it is now, your code makes a copy of the string m_value because you return by value, which is not as good as only returning a const reference.

If you provide a complete code sample that shows the problem, I could help you better.

Answer 3

not that it matters but that std::string return in that class can use a const else wise you're just creating a copy of the member value which is a waste.

std::string value() const { return m_value; }

Answer 4

Here's what's typically happening when you write:

printf("value: %s\n", v1.value().c_str());

1. The compiler makes a temporary std::string to hold the value returned from v1.value().
2. It calls v1.value() and puts its return value in the temporary string (exactly how it does this can vary: typically it will pass a reference to the temporary as a hidden parameter to the method. See http://en.wikipedia.org/wiki/Return_value_optimization for discussion).
3. It calls .c_str() on the temporary std::string, it stashes the const char * result away somewhere (e.g. a register).
4. It's now finished with the temporary std::string, so destroys it (i.e. calls its destructor, maybe frees some stack space).
5. It passes the const char * pointer it got in step (3) as an argument to printf().

The problem is that the pointer in step 3 points to memory allocated by the temporary std::string, which gets freed when the temporary's destructor is called. This memory can be long gone by the time it gets used by printf().

Basically, any usage like the one you've shown is dangerous and should be avoided. Using the following is correct:

std::string copiedString = v1.value();
printf("value: %s\n", copiedString.c_str());

because the destructor for copiedString doesn't get called until copiedString goes out of scope, some time after printf() has returned. In fact, this is no less efficient than v1.value().c_str(), because a temporary std::string gets created in either case.

Returning a reference to a string is a fine alternative, provided that the reference remains valid for as long as the caller needs it to. So, a reference to a member variable in a long-lived object is OK; a reference to something which turns out to be temporary isn't.