Another C++ learning moment: returning strings from functions

Question

I've got some basic questions about C++. Consider the following code in which I attempt to return a string.

const std::string&
NumberHolder::getValueString() {
    char valueCharArray[100];
    sprintf_s(valueCharArray,"%f",_value);
    std::string valueString(valueCharArray);
    return valueString;
}

I'm attempting to return a string with the value of a class member called _value. However I'm getting the warning that I'm trying to pass back a pointer to a local variable. This is of course a bad thing. If I understand C++ enough at this point, this means that the pointer I pass back will already have delete called on it by the time someone tries to use it. So I modify:

const std::string&
NumberHolder::getValueString() {
    char valueCharArray[100];
    sprintf_s(valueCharArray,"%f",_value);
    std::string valueString = new std::string(valueCharArray);
    return (*valueString);
}

This should create a pointer on the stack which will survive outside of this function. Two problems here though: 1) it doesn't compile anyway and I don't understand why (error = cannot convert from 'std::string *' to 'std::basic_string<_Elem,_Traits,_Ax>') and 2) This seems like a potential memory leak because I'm depending upon someone else to call delete on this guy. What pattern should I be using here?

Solution 1

You're defeating the point of having a std::string by allocating it on the heap!

Just return it by value like this:

std::string NumberHolder::getValueString()
{ 
    char valueCharArray[100]; 
    sprintf_s(valueCharArray,"%f",_value); 
    return std::string(valueCharArray); 
} 

Just about every compiler nowadays will do return value optimization (RVO) on the return statement, so no copies should be made. Consider the following:

NumberHolder holder;
// ...
std::string returnedString = holder.getValueString();

With RVO, the compiler will generate the code for the above implementation of NumberHolder::getValueString() such that std::string is constructed at the location of returnedString, so no copies are needed.

Solution 2

You are getting this warning because you are returning a reference to the local string, not a copy of the local string. Once the function returns, the local string is destroyed and the reference that you returned is invalid. Thus, you need to return the string by value, not by reference:

std::string NumberHolder::getValueString()

Solution 3

Your 1st attempt is correct if you return a temp variable but bind it in a const reference.

const std::string NumberHolder::getValueString(){}

const std::string& val = NumberHolder::getValueString();

const . But your second attempt is dangerous, depending on somebody else to delete.

Comments

[Kate Gregory]

It's not about returning a string; it's about returning a reference. A dangling reference.

[Admin]

Besides your reference issues.. Why const anyways?

[Fanatic23]

std::string valueString = new std::string(valueCharArray); return (valueString); You forgot to make valueString a pointer, that's why doesn't compile: std::string valueString = new std::string(valueCharArray);

[JnBrymn]

Any issues there with a memory leak? The calling function would have to take responsibility for deleting the string, right? This doesn't seem like the best thing.

[In silico]

std::string takes care of that for you.

[James McNellis]

@John: Don't create the std::string on the heap at all. Just return the local string by value. Your first example is fine; just change the return type.

[JnBrymn]

Ok... this is something I could learn from. What's happening here? I think that I'm making an anonymously named string in the scope of the function and then by passing it back by value I'm effectively making a copy of it that lives in the scope above the call to this function. Is that correct? UPDATE: ok, I get it now.

[In silico]

Yes, you are creating an anonymous std::string, but compilers actually optimize for this specific case and will eliminate the copy by constructing the std::string directly on the location of the returned value.

[nobody]

No leak. Worst case, a temporary will be created for the return, copied to the caller's variable, then destructed (and the caller's copy destructed when it goes out of scope). Fancier implementations may be able to avoid the copying.

[Philipp]

Some people argue that you should return const std::string.