Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Return for move-only return types. #330

Merged
merged 1 commit into from
Apr 20, 2024
Merged

Conversation

FranckRJ
Copy link
Collaborator

@FranckRJ FranckRJ commented Apr 14, 2024

Now the object passed to Return(o) is always move-returned if it is passed by rvalue reference, even if it can by copied. The goal is that if the user pass it by std::move they probably don't want it to be copied (maybe it can increase a counter somewhere and they don't want it to) and it will also fix issues with types where you can't properly detect if they're copyable or not like std::vector.

Tests should be added to ensure this behavior (object is not copied even if it has a copy constructor).

@malcolmdavey Do you think this behavior is OK or do you see an issue with it ?

This is only supported by the Return function that takes a single object, it should be supported by Return(o1, o2, o3).

A very strange thing happened while implementing that feature, it seems the template <typename U = T> trick doesn't work well with std::is_reference: https://godbolt.org/z/hjfKaq1dW

The compiler complains that func(T&& t) conflicts with func(const T& t). It's true that they conflict, because T = int& so once we replace T we get something like func(T & && t) and func(T & const & t) which collapse to func(T & t) and func(T & t). The issue is that func(T&& t) should be disabled by SFINAE because T is a reference, so it shouldn't cause an error, and it's what happen when using std::is_copy_constructible, so why doesn't it work with std::is_reference ?

Because this trick doesn't work I used the "inherit partially specialized class" method instead.

Should fix #327.

@FranckRJ FranckRJ added this to the 2.5.0 milestone Apr 14, 2024
@coveralls
Copy link

coveralls commented Apr 14, 2024

Coverage Status

coverage: 99.926%. remained the same
when pulling 7825ac7 on better-return-move-only
into fa53ef2 on dev.

@malcolmdavey
Copy link
Contributor

Not an expert in the implementation approach, so can't really comment more. It's ultimately about implementing something that works and makes sense, as this is a tool. (and haven't had a chance to progress that other open PR yet).

The scenario makes sense that it should work for Return (but of course can't work for AlwaysReturn)

@malcolmdavey
Copy link
Contributor

There are also std::is_rvalue_reference, and std::is_lvalue_reference, but not sure if they would work nor be the best approach either.

A critical bit is to have enough test coverage, in case we need to change the implementation of course .

@FranckRJ
Copy link
Collaborator Author

It's ultimately about implementing something that works and makes sense

Yeah, I guess that moving a value if you're explicitly calling std::move on it is the behavior the makes the most sense, even when it's a copyable value. I'll go with that.

(and haven't had a chance to progress that other open PR yet).

If you're talking about #310 / #280 I've been a bit lazy with it, I'll look into merging it this weekend (for real, I just need to remember how it works because I've forgotten since the last time I read it), I'll fix the conflicts myself if there's any don't worry.

@FranckRJ FranckRJ force-pushed the better-return-move-only branch 2 times, most recently from f2a2cd8 to 95b2585 Compare April 20, 2024 13:47
@FranckRJ FranckRJ force-pushed the better-return-move-only branch from 95b2585 to 7825ac7 Compare April 20, 2024 13:55
@FranckRJ FranckRJ merged commit 1ed5f80 into dev Apr 20, 2024
62 checks passed
@FranckRJ FranckRJ deleted the better-return-move-only branch April 20, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants