Why give a C++ compiler warning when returning an rvalue reference?

Question

I've been studying rvalue references (a new concept for me), and am puzzled by a warning I receive in the following class function...

string&& Sampler::Serial() const {
    stringstream ss;
    .
    . [assemble a string value using data members]
    .
    return ss.str();
}

This compiles successfully, but with the following warning...

..\Metrics\Sampler.cpp:71:16: warning: returning reference to temporary [-Wreturn-local-addr]
  return ss.str();
                ^

I'm fully aware that I'm returning a temporary, as evidenced by the fact that I'm using an rvalue reference as my return type. The code seems to run fine upon execution, so why should this warrant a compiler warning?

The standard answer to similar questions seems to be to copy the return value instead of using a reference, but why should I copy potentially massive amounts of temporary data when I can move it with an rvalue reference? Isn't that why it was invented?

rvalue references are still references; there's no magical lifetime extension for the temporary, which is destroyed before control returns to the caller of `Serial()`, so the reference you return is always dangling. — T.C., Jul 03 '15 at 10:48
You are not returning a temporary; you are returning a reference to a temporary. Nor are you moving anything. — M.M, Jul 03 '15 at 10:49
I'm not sure I see the problem that wasn't discussed in Johanne's answer to the linked question. You're not "copying potentially massive amounts" of *anything* if you simple return `std::string` and `ss.str()`. What makes you think move semantics will somehow *not* apply if properly invoked? — WhozCraig, Jul 03 '15 at 10:50
@T.C. If that were the case, I would think the code wouldn't successfully return the value it's intended to; which it does. — Syndog, Jul 03 '15 at 10:51
@Syndog Undefined behaviour has the nasty result of usually doing what you think it should do, until it doesn't. — , Jul 03 '15 at 10:52
@WhozCraig Correct me if I'm wrong, but doesn't switching the return type from a reference to a non-reference necessitate copying the value rather than passing it by reference? — Syndog, Jul 03 '15 at 10:55
@Syndog `std::string` supports move-construction; you're going to move from the rvalue returned by `str()` into your res, and move from *that* into your caller's target. `std::string` is optimized for allowing move-assignment and move-construction. In short, you're pretty-much going to have to jump through hoops to force this *not* to do what you want if you simply use `std::string` and `return ss.str();`. — WhozCraig, Jul 03 '15 at 11:00
@hvd So what you're saying is, the "successful" results I'm seeing are an artifact of residual values left over in memory that's already been deallocated, and therefore unreliable? That makes sense. — Syndog, Jul 03 '15 at 11:05
Thank you all for your comments and answers. This is been invaluable! — Syndog, Jul 03 '15 at 11:10
@Syndog Yes, exactly. That's why the compiler warns as well: returning a reference to a function-local object (including a temporary) is never a good idea. — , Jul 03 '15 at 11:11
@WhozCraig In other words, don't presume either copy or move semantics until you hit the assignment operator / constructor. Thaaat's the mistake I made. Thank you! — Syndog, Jul 03 '15 at 11:33

score 8 · Accepted Answer · 2015-07-03T10:58:50.740

8

You're not moving your data. You're creating a local object, creating a reference to that local object, destroying that local object, and then still using that reference.

You should return by value, as you already found. But instead of copying, move the data. That's the safe way of ensuring you don't copy those massive amounts of data.

std::string Sampler::Serial() const {
    std::stringstream ss;
    .
    . [assemble a string value using data members]
    .
    return std::move(ss.str());
}

Note: the std::move is technically redundant here, as ss.str() already returns an rvalue and so would already be moved. I recommend leaving it in anyway. This way works in any situation, so you don't have to think about which form to use: if you want to move, write move.

As pointed out by T.C., in general, though not in your case, this can prevent RVO. In cases where RVO is possible and where the compiler would implicitly use a move anyway, there is no need to write move explicitly. For instance:

std::string f() {
  std::string x;
  ...
  return x; // not std::move(x)
}

Here, it should already be clear to the reader that x is a local variable. It's normal for C++ code to return local variables without writing move, because either the compiler will elide the x local variable entirely and construct the std::string object directly in the return slot (whatever that means for your platform), or the compiler will use the move constructor of std::string implicitly anyway.

edited Jul 03 '15 at 10:58

answered Jul 03 '15 at 10:49

6

That inhibits RVO though. – T.C. Jul 03 '15 at 10:53
@T.C. RVO is already not possible here, but fair point. In cases where RVO would otherwise be possible, don't write `move`. Will edit. – Jul 03 '15 at 10:54
1

@hvd: This will still prevent copy-elision, AFAIK. – Xeo Jul 03 '15 at 11:15
@Xeo Hmm. Yeah, I think you're right, it will... Darn. Fundamentally changing my answer now would not be appropriate, it would misrepresent the votes that have already been cast (or so I've been told, anyway), but if you want to write up a different answer that explains why *not* to write the `std::move` here, you'll get an upvote from me and the OP can then hopefully accept that instead. – Jul 03 '15 at 11:19
@hvd Much agreed. I'll keep an eye out. – Syndog Jul 03 '15 at 11:27
@hvd I meant RVO, not NRVO. The temporary returned by `str()` can be constructed directly into the return value of `Serial()`. – T.C. Jul 04 '15 at 01:18
@T.C. Yeah, somehow I didn't manage to get that from your comment, but did from Xeo's, even though in retrospect your comment seems perfectly clear. My response to Xeo also applies to you, then. – Jul 04 '15 at 06:09

score 5 · Answer 2 · answered Jul 03 '15 at 10:49

This is analogous to returning an lvalue reference to a local variable and puts you on the fast-track to undefined behaviour.

Regardless of whether you return an lvalue reference or an rvalue reference, you are still referencing memory which is going to be destroyed on function exit.

Rvalue reference return types should be reserved for cases when you are referencing an object which has a lifetime longer than the function, but you don't need it anymore, so are fine with it being moved-from for efficiency. For example, you might have a case in which you are temporarily storing some data, but clients can choose to "steal" the data from you. Returning an rvalue reference to the data would be reasonable in that case.

Giving this a +1 for excellent plain-english exposition. Thanks, TartanLlama! — Syndog, Jul 03 '15 at 11:16

Jarod42 · Answer 3 · 2015-07-03T10:49:58.407

4

You return a (rvalue) reference to object (return by str()) of temporary object (ss) which is destroyed at end of scope.

You should return object instead:

string Sampler::Serial() const

edited Jul 03 '15 at 10:49

answered Jul 03 '15 at 10:49

Jarod42

203,559
14
181
302

Why give a C++ compiler warning when returning an rvalue reference?

3 Answers3

Linked