Whilst refactoring some code I came across some getter methods that returns a std::string. Something like this for example:
class foo
{
private:
std::string name_;
public:
std::string name()
{
return name_;
}
};
Surely the getter would be better returning a const std::string&? The current method is returning a copy which isn't as efficient. Would returning a const reference instead cause any problems?
The only way this can cause a problem is if the caller stores the reference, rather than copy the string, and tries to use it after the object is destroyed. Like this:
foo *pFoo = new foo;
const std::string &myName = pFoo->getName();
delete pFoo;
cout << myName; // error! dangling reference
However, since your existing function returns a copy, then you would not break any of the existing code.
Edit: Modern C++ (i. e. C++11 and up) supports Return Value Optimization, so returning things by value is no longer frowned upon. One should still be mindful of returning extremely large objects by value, but in most cases it should be ok.
Actually, another issue specifically with returning a string not by reference, is the fact that std::string provides access via pointer to an internal const char* via the c_str() method. This has caused me many hours of debugging headache. For instance, let's say I want to get the name from foo, and pass it to JNI to be used to construct a jstring to pass into Java later on, and that name() is returning a copy and not a reference. I might write something like this:
foo myFoo = getFoo(); // Get the foo from somewhere.
const char* fooCName = foo.name().c_str(); // Woops! foo.name() creates a temporary that's destructed as soon as this line executes!
jniEnv->NewStringUTF(fooCName); // No good, fooCName was released when the temporary was deleted.
If your caller is going to be doing this kind of thing, it might be better to use some type of smart pointer, or a const reference, or at the very least have a nasty warning comment header over your foo.name() method. I mention JNI because former Java coders might be particularly vulnerable to this type of method chaining that may seem otherwise harmless.
One problem for the const reference return would be if the user coded something like:
const std::string & str = myObject.getSomeString() ;
With a std::string return, the temporary object would remain alive and attached to str until str goes out of scope.
But what happens with a const std::string &? My guess is that we would have a const reference to an object that could die when its parent object deallocates it:
MyObject * myObject = new MyObject("My String") ;
const std::string & str = myObject->getSomeString() ;
delete myObject ;
// Use str... which references a destroyed object.
So my preference goes to the const reference return (because, anyway, I'm just more confortable with sending a reference than hoping the compiler will optimize the extra temporary), as long as the following contract is respected: "if you want it beyond my object's existence, they copy it before my object's destruction"
Some implementations of std::string share memory with copy-on-write semantics, so return-by-value can be almost as efficient as return-by-reference and you don't have to worry about the lifetime issues (the runtime does it for you).
If you're worried about performance, then benchmark it (<= can't stress that enough) !!! Try both approaches and measure the gain (or lack thereof). If one is better and you really care, then use it. If not, then prefer by-value for the protection it offers agains lifetime issues mentioned by other people.
You know what they say about making assumptions...
Okay, so the differences between returning a copy and returning the reference are:
Performance: Returning the reference may or may not be faster; it depends on how std::string is implemented by your compiler implementation (as others have pointed out). But even if you return the reference the assignment after the function call usually involves a copy, as in std::string name = obj.name();
Safety: Returning the reference may or may not cause problems (dangling reference). If the users of your function don't know what they are doing, storing the reference as reference and using it after the providing object goes out of scope then there's a problem.
If you want it fast and safe use boost::shared_ptr. Your object can internally store the string as shared_ptr and return a shared_ptr. That way, there will be no copying of the object going and and it's always safe (unless your users pull out the raw pointer with get() and do stuff with it after your object goes out of scope).
I'd change it to return const std::string&. The caller will probably make a copy of the result anyway if you don't change all the calling code, but it won't introduce any problems.
One potential wrinkle arises if you have multiple threads calling name(). If you return a reference, but then later change the underlying value, then the caller's value will change. But the existing code doesn't look thread-safe anyway.
Take a look at Dima's answer for a related potential-but-unlikely problem.
It is conceivable that you could break something if the caller really wanted a copy, because they were about to alter the original and wanted to preserve a copy of it. However it is far more likely that it should, indeed, just be returning a const reference.
The easiest thing to do is try it and then test it to see if it still works, provided that you have some sort of test you can run. If not, I'd focus on writing the test first, before continuing with refactoring.
Odds are pretty good that typical usage of that function won't break if you change to a const reference.
If all of the code calling that function is under your control, just make the change and see if the compiler complains.
Does it matter? As soon as you use a modern optimizing compiler, functions that return by value will not involve a copy unless they are semantically required to.
See the C++ lite FAQ on this.
Depends what you need to do. Maybe you want to all the caller to change the returned value without changing the class. If you return the const reference that won't fly.
Of course, the next argument is that the caller could then make their own copy. But if you know how the function will be used and know that happens anyway, then maybe doing this saves you a step later in code.
I normally return const& unless I can't. QBziZ gives an example of where this is the case. Of course QBziZ also claims that std::string has copy-on-write semantics which is rarely true today since COW involves a lot of overhead in a multi-threaded environment. By returning const & you put the onus on the caller to do the right thing with the string on their end. But since you are dealing with code that is already in use you probably shouldn't change it unless profiling shows that the copying of this string is causing massive performance problems. Then if you decide to change it you will need to test thouroughly to make sure you didn't break anything. Hopefully the other developers you work with don't do sketchy stuff like in Dima's answer.
Returning a reference to a member exposes the implementation of the class. That's could prevent to change the class. May be usefull for private or protected methods incase the optimization is needed. What should a C++ getter return
