Prevent using ReturnRef on reference to temporary

[PiotrNycz]

Fixed issue: 2471

[sbenzaquen]

Please include a test that triggers the bug and gets fixed with this change.

[kuzkry]

How is he going to test non-compiling code? It isn't run-time.

[sbenzaquen]

You can use decltype testing to evaluate the validity of an expression.
Similar to how std::is_detected is meant to work (but we can't use that because it is not available in C++11).

Example:

template <typename T, typename = decltype(ReturnRef(std::declval<T&&>()))>
bool CanCallReturnRef(T&&) { return true; }
bool CanCallReturnRef(Unused) { return false; }

...
int i = 0;
EXPECT_TRUE(CanCallReturnRef(i));
EXPECT_FALSE(CanCallReturnRef(i + 1));

[kuzkry]

Ah, ok. I know this trick. Though I think the standard way of doing this is by using void_t, isn't it?

[sbenzaquen]

Ah, ok. I know this trick. Though I think the standard way of doing this is by using void_t, isn't it?

void_t is not available in C++11, but you can do without it too.

[PiotrNycz]

"POD" is not really a relevant concept since C++11.
Also, this bug was not related to pod-ness afaik. POD structs would still have triggered the bug.

You are right. The difference is between basic (integral) types and more complex classes. I'll change that (the names).

[PiotrNycz]

This is redundant with CanCallReturnRef.
We should only have one.
Is HasReturnRefAction giving something that CanCallReturnRef wasn't?

IMHO HasReturnRefAction adding more information to these tests - it is somehow easier to get the difference. But if you think it is redundant I can remove it, no problem at all.

[sbenzaquen]

This is redundant with CanCallReturnRef.
We should only have one.
Is HasReturnRefAction giving something that CanCallReturnRef wasn't?

IMHO HasReturnRefAction adding more information to these tests - it is somehow easier to get the difference. But if you think it is redundant I can remove it, no problem at all.

I see it as a different way of asking the same question.

[PiotrNycz]

This is redundant with CanCallReturnRef.
We should only have one.
Is HasReturnRefAction giving something that CanCallReturnRef wasn't?

IMHO HasReturnRefAction adding more information to these tests - it is somehow easier to get the difference. But if you think it is redundant I can remove it, no problem at all.

I see it as a different way of asking the same question.

Ok, I'll remove HasReturnRefAction with next commit.

[kuzkry]

Do we really need 6 commits for that? :D I suggest squashing them all into one.

[gennadiycivil]

Thank you, we have started internal review. Please don't push any more changes into this PR as they might be overwritten.
(276737278)