eranpeer / FakeIt

C++ mocking made easy. A simple yet very expressive, headers only library for c++ mocking.
MIT License
1.22k stars 170 forks source link

Improve Return for move-only return types. #330

Closed FranckRJ closed 2 months ago

FranckRJ commented 2 months ago

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.

coveralls commented 2 months ago

Coverage Status

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

malcolmdavey commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.