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.

Sign up for GitHub

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

Oss 7 copy return for return by ref methods #310

Closed
wants to merge 11 commits into from
+138 −9
Closed

Oss 7 copy return for return by ref methods #310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4deaddc
Take copy or return value if explicit by value used, when mocking ret…
malcolmdavey Jun 20, 2022
f400443
fix typos
malcolmdavey Jun 21, 2022
6820aeb
Handle automatically for temporaries
malcolmdavey Jun 22, 2022
fce1dd8
Remove optional move (which was just an optimisation), so we can supp…
malcolmdavey Jun 27, 2022
4e91eef
Add ReturnCopy
malcolmdavey Jul 18, 2022
e856bee
Always Return Copy
malcolmdavey Jul 19, 2022
22cbe76
Merge branch 'dev-2.3.1' into OSS-7-copy-return-for-return-by-ref-met…
malcolmdavey Jul 19, 2022
deb9714
All RRef for AlwaysReturn for Ref args
malcolmdavey Jul 19, 2022
4400bdd
Merge branch 'OSS-7-copy-return-for-return-by-ref-methods' of github.…
malcolmdavey Jul 19, 2022
a3b6b2d
Fix up to work with newer move only return tests.
malcolmdavey Jul 19, 2022
36ad3e6
Fix mistake in new AlwaysReturn
malcolmdavey Jul 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions include/fakeit/ActionSequence.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@ namespace fakeit {
Action<R, arglist...> &action = dynamic_cast<Action<R, arglist...> &>(destructable);
std::function<void()> finallyClause = [&]() -> void {
if (action.isDone())
{
_recordedActions.erase(_recordedActions.begin());
_usedActions.push_back(destructablePtr);
}
};
Finally onExit(finallyClause);
return action.invoke(args);
Expand Down Expand Up @@ -81,6 +84,7 @@ namespace fakeit {
}

std::vector<std::shared_ptr<Destructible>> _recordedActions;
std::vector<std::shared_ptr<Destructible>> _usedActions;
};

}
38 changes: 35 additions & 3 deletions include/fakeit/StubbingProgress.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,17 @@ namespace fakeit {
template<typename U = R>
typename std::enable_if<!std::is_reference<U>::value, MethodStubbingProgress<R, arglist...> &>::type
Return(const R &r) {
return Do([r](const typename fakeit::test_arg<arglist>::type...) -> R { return r; });
return Do([r](const typename fakeit::test_arg<arglist>::type...) mutable -> R { return r; });
}

template<typename T>
typename std::enable_if<!std::is_reference<T>::value && std::is_copy_constructible<T>::value, MethodStubbingProgress<R, arglist...>&>::type
Return(T&& t) {
Comment on lines +56 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I'm getting failures for this line/test: cannot convert argument 1 from 'int' to 'int &'

 			When(Method(mock, returnIntByRef)).Return(1);
Suggested change
template<typename T>
typename std::enable_if<!std::is_reference<T>::value && std::is_copy_constructible<T>::value, MethodStubbingProgress<R, arglist...>&>::type
Return(T&& t) {
template<typename U = R>
typename std::enable_if<!std::is_reference<T>::value && std::is_copy_constructible<T>::value, MethodStubbingProgress<R, arglist...>&>::type
Return(R&& r) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "currently" you mean the code in the PR (so, without the changes in your message) or the code once you apply the changes in your message?

Copy link
Contributor

@malcolmdavey malcolmdavey Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, if I try this "suggested change" to the PR, then it fails for basic integer values (maybe because of other overloads)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this

        typename std::enable_if<!std::is_reference<T>::value && std::is_copy_constructible<T>::value, MethodStubbingProgress<R, arglist...>&>::type
    	Return(R&& r) {

I get this error message:

referece_types_tests.cpp
c:\devel\git\fakeit\tests\referece_types_tests.cpp(196): error C2664: 'fakeit::MethodStubbingProgress &fakeit::MethodStubbingProgress::Return(void)': cannot convert argument 1 from 'int' to 'int &'
with
[
R=int &
]

Not saying there isn't a way to fix it but just haven't found it yet.

Also thinking the existing overload just shouldn't support R value refs at all, so should be specific to lvalue refs, in stead of all refs, as it will always be a dangling pointer, and hence always undefined behaviour.

auto store = std::make_shared<T>(std::move(t));
return Do([store](const typename fakeit::test_arg<arglist>::type...) mutable->R
{
return *store;
});
}

template<typename U = R>
Expand All @@ -61,11 +71,17 @@ namespace fakeit {

template<typename U = R>
typename std::enable_if<!std::is_copy_constructible<U>::value, MethodStubbingProgress<R, arglist...>&>::type
Return(R&& r) {
Return(R&& r) {
auto store = std::make_shared<R>(std::move(r)); // work around for lack of move_only_funciton( C++23) - move into a shared_ptr which we can copy.
return Do([store](const typename fakeit::test_arg<arglist>::type...) mutable -> R {
return std::move(*store);
});
});
}

template<typename U = R>
typename std::enable_if<std::is_copy_constructible<U>::value, MethodStubbingProgress<R, arglist...>&>::type
ReturnCopy(const R& r) {
return Do([r](const typename fakeit::test_arg<arglist>::type...) mutable -> R { return r; });
}

MethodStubbingProgress<R, arglist...> &
Expand All @@ -89,12 +105,28 @@ namespace fakeit {
return AlwaysDo([r](const typename fakeit::test_arg<arglist>::type...) -> R { return r; });
}

template<typename T>
typename std::enable_if<!std::is_reference<T>::value && std::is_copy_constructible<T>::value, void>::type
AlwaysReturn(T&& t) {
auto store = std::make_shared<T>(std::move(t));
return AlwaysDo([store](const typename fakeit::test_arg<arglist>::type...) mutable -> R
{
return *store;
});
}

template<typename U = R>
typename std::enable_if<std::is_reference<U>::value, void>::type
AlwaysReturn(const R &r) {
return AlwaysDo([&r](const typename fakeit::test_arg<arglist>::type...) -> R { return r; });
}

template<typename U = R>
typename std::enable_if<std::is_copy_constructible<U>::value, void>::type
AlwaysReturnCopy(const R& r) {
return AlwaysDo([r](const typename fakeit::test_arg<arglist>::type...) mutable -> R { return r; });
}

MethodStubbingProgress<R, arglist...> &
Return() {
return Do([](const typename fakeit::test_arg<arglist>::type...) -> R { return DefaultValue<R>::value(); });
Expand Down
105 changes: 99 additions & 6 deletions tests/referece_types_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ struct ReferenceTypesTests: tpunit::TestFixture {
class ConcreteType: public AbstractType {
public:
int state;
ConcreteType() :
state(10) {
ConcreteType(int value = 10) :
state(value) {
}
virtual void foo() override {
}
Expand All @@ -40,15 +40,21 @@ struct ReferenceTypesTests: tpunit::TestFixture {
virtual int& returnIntByRef() = 0;
virtual AbstractType& returnAbstractTypeByRef() = 0;
virtual ConcreteType& returnConcreteTypeByRef() = 0;
virtual const std::string& returnStringByConstRef() = 0;
virtual std::string& returnStringByRef() = 0;
};

ReferenceTypesTests() :
tpunit::TestFixture(
//
TEST(ReferenceTypesTests::implicitStubbingDefaultReturnValues),
TEST(ReferenceTypesTests::explicitStubbingDefualtReturnValues),
TEST(ReferenceTypesTests::explicitStubbingDefualtReturnValues_with_AlwaysReturn),
TEST(ReferenceTypesTests::explicitStubbingDefaultReturnValues),
TEST(ReferenceTypesTests::explicitStubbingReturnCopyValuesForRef),
TEST(ReferenceTypesTests::explicitStubbingReturnValuesCopyForRRef),
TEST(ReferenceTypesTests::explicitStubbingDefaultReturnValues_with_AlwaysReturn),
TEST(ReferenceTypesTests::explicitStubbingReturnValues_with_AlwaysReturn),
TEST(ReferenceTypesTests::explicitStubbingReturnCopyValuesForRef_with_AlwaysReturn),
TEST(ReferenceTypesTests::explicitStubbingReturnCopyValues_with_AlwaysReturn),
TEST(ReferenceTypesTests::explicitStubbingReturnValues_by_assignment),
TEST(ReferenceTypesTests::explicitStubbingReturnValues)
//
Expand Down Expand Up @@ -77,7 +83,7 @@ struct ReferenceTypesTests: tpunit::TestFixture {
ASSERT_EQUAL(nullptr, &i.returnAbstractTypeByRef());
}

void explicitStubbingDefualtReturnValues() {
void explicitStubbingDefaultReturnValues() {
Mock<ReferenceInterface> mock; //
When(Method(mock,returnIntByRef)).Return(); //
When(Method(mock,returnAbstractTypeByRef)).Return(); //
Expand Down Expand Up @@ -121,6 +127,66 @@ struct ReferenceTypesTests: tpunit::TestFixture {
ASSERT_EQUAL(&c, &i.returnAbstractTypeByRef());
}

void explicitStubbingReturnCopyValuesForRef() {
Mock<ReferenceInterface> mock;

// add scope so we know we are copying
{
std::string a_string{"myString"};
int num{ 1 };

// explicit copy here
When(Method(mock, returnStringByConstRef)).ReturnCopy(a_string);
When(Method(mock, returnIntByRef)).ReturnCopy(num);

// modify now so know whether or not is was copied
a_string = "modified";
num = 2;
}

ReferenceInterface& i = mock.get();

// Fundamental types are initiated to 0.
EXPECT_EQUAL("myString", i.returnStringByConstRef());
EXPECT_EQUAL(1, i.returnIntByRef());
}

void explicitStubbingReturnValuesCopyForRRef() {
Mock<ReferenceInterface> mock;

{
When(Method(mock, returnStringByConstRef)).Return(std::string{ "myConstRefString" });
When(Method(mock, returnStringByRef)).Return(std::string{ "myRefString" });
When(Method(mock, returnConcreteTypeByRef)).Return(ConcreteType(20));
When(Method(mock, returnIntByRef)).Return(1);
}

ReferenceInterface& i = mock.get();

EXPECT_EQUAL("myConstRefString", i.returnStringByConstRef());
EXPECT_EQUAL("myRefString", i.returnStringByRef());
EXPECT_EQUAL(ConcreteType(20), i.returnConcreteTypeByRef());
EXPECT_EQUAL(1, i.returnIntByRef());
}

void explicitStubbingReturnCopyValuesForRef_with_AlwaysReturn() {
Mock<ReferenceInterface> mock;

{
When(Method(mock, returnStringByConstRef)).AlwaysReturn(std::string{ "myConstRefString" });
When(Method(mock, returnStringByRef)).AlwaysReturn(std::string{ "myRefString" });
When(Method(mock, returnConcreteTypeByRef)).AlwaysReturn(ConcreteType(20));
When(Method(mock, returnIntByRef)).AlwaysReturn(1);
}

ReferenceInterface& i = mock.get();

EXPECT_EQUAL("myConstRefString", i.returnStringByConstRef());
EXPECT_EQUAL("myRefString", i.returnStringByRef());
EXPECT_EQUAL(ConcreteType(20), i.returnConcreteTypeByRef());
EXPECT_EQUAL(1, i.returnIntByRef());
}

void explicitStubbingReturnValues_with_AlwaysReturn() {
Mock<ReferenceInterface> mock; //

Expand All @@ -144,6 +210,33 @@ struct ReferenceTypesTests: tpunit::TestFixture {
// For abstract types return a reference to nullptr.
ASSERT_EQUAL(&c, &i.returnAbstractTypeByRef());
}

void explicitStubbingReturnCopyValues_with_AlwaysReturn() {
Mock<ReferenceInterface> mock;

// add scope so we know we are copying
{
std::string a_string{ "myString" };
int num{ 1 };

// explicit copy here
When(Method(mock, returnStringByConstRef)).AlwaysReturnCopy(a_string);
When(Method(mock, returnIntByRef)).AlwaysReturnCopy(num);

// modify now so know whether or not is was copied
a_string = "modified";
num = 2;
}

ReferenceInterface& i = mock.get();

// Fundamental types are initiated to 0.
EXPECT_EQUAL("myString", i.returnStringByConstRef());
EXPECT_EQUAL("myString", i.returnStringByConstRef());

EXPECT_EQUAL(1, i.returnIntByRef());
EXPECT_EQUAL(1, i.returnIntByRef());
}

void explicitStubbingReturnValues_by_assignment() {
Mock<ReferenceInterface> mock; //
Expand All @@ -169,7 +262,7 @@ struct ReferenceTypesTests: tpunit::TestFixture {
ASSERT_EQUAL(&c, &i.returnAbstractTypeByRef());
}

void explicitStubbingDefualtReturnValues_with_AlwaysReturn() {
void explicitStubbingDefaultReturnValues_with_AlwaysReturn() {
Mock<ReferenceInterface> mock;
When(Method(mock,returnIntByRef)).AlwaysReturn();
When(Method(mock,returnAbstractTypeByRef)).AlwaysReturn(); //
Expand Down