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

Fix MOCK_METHOD to handle noexcept correctly #2498

Merged
merged 6 commits into from
Oct 11, 2019
Merged

Fix MOCK_METHOD to handle noexcept correctly #2498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
10c1d8c
Use the verbatim noexcept spec in MOCKED_METHOD
thejcannon Oct 7, 2019
0eadff8
Fix spacing
thejcannon Oct 7, 2019
872b9ce
Avoid comma operator
thejcannon Oct 7, 2019
7f8617a
Switch to free function to avoid GCC bug
thejcannon Oct 7, 2019
20255e6
Use declval in noexcept expression
thejcannon Oct 7, 2019
bc996e0
Made noexcept condition more exciting
thejcannon Oct 10, 2019
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
24 changes: 14 additions & 10 deletions googlemock/include/gmock/gmock-function-mocker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@
GMOCK_INTERNAL_MOCK_METHOD_IMPL( \
GMOCK_PP_NARG0 _Args, _MethodName, GMOCK_INTERNAL_HAS_CONST(_Spec), \
GMOCK_INTERNAL_HAS_OVERRIDE(_Spec), GMOCK_INTERNAL_HAS_FINAL(_Spec), \
GMOCK_INTERNAL_HAS_NOEXCEPT(_Spec), GMOCK_INTERNAL_GET_CALLTYPE(_Spec), \
GMOCK_INTERNAL_GET_NOEXCEPT_SPEC(_Spec), \
GMOCK_INTERNAL_GET_CALLTYPE(_Spec), \
(GMOCK_INTERNAL_SIGNATURE(_Ret, _Args)))

#define GMOCK_INTERNAL_MOCK_METHOD_ARG_5(...) \
Expand Down Expand Up @@ -100,15 +101,16 @@
GMOCK_PP_FOR_EACH(GMOCK_INTERNAL_ASSERT_VALID_SPEC_ELEMENT, ~, _Spec)

#define GMOCK_INTERNAL_MOCK_METHOD_IMPL(_N, _MethodName, _Constness, \
_Override, _Final, _Noexcept, \
_Override, _Final, _NoexceptSpec, \
_CallType, _Signature) \
typename ::testing::internal::Function<GMOCK_PP_REMOVE_PARENS( \
_Signature)>::Result \
GMOCK_INTERNAL_EXPAND(_CallType) \
_MethodName(GMOCK_PP_REPEAT(GMOCK_INTERNAL_PARAMETER, _Signature, _N)) \
GMOCK_PP_IF(_Constness, const, ) GMOCK_PP_IF(_Noexcept, noexcept, ) \
GMOCK_PP_IF(_Override, override, ) \
GMOCK_PP_IF(_Final, final, ) { \
GMOCK_PP_IF(_Constness, const, ) \
_NoexceptSpec \
GMOCK_PP_IF(_Override, override, ) \
GMOCK_PP_IF(_Final, final, ) { \
GMOCK_MOCKER_(_N, _Constness, _MethodName) \
.SetOwnerAndName(this, #_MethodName); \
return GMOCK_MOCKER_(_N, _Constness, _MethodName) \
Expand All @@ -125,7 +127,7 @@
const ::testing::internal::WithoutMatchers&, \
GMOCK_PP_IF(_Constness, const, )::testing::internal::Function< \
GMOCK_PP_REMOVE_PARENS(_Signature)>*) \
const GMOCK_PP_IF(_Noexcept, noexcept, ) { \
const _NoexceptSpec { \
return GMOCK_PP_CAT(::testing::internal::AdjustConstness_, \
GMOCK_PP_IF(_Constness, const, ))(this) \
->gmock_##_MethodName(GMOCK_PP_REPEAT( \
Expand All @@ -147,9 +149,12 @@
#define GMOCK_INTERNAL_HAS_FINAL(_Tuple) \
GMOCK_PP_HAS_COMMA(GMOCK_PP_FOR_EACH(GMOCK_INTERNAL_DETECT_FINAL, ~, _Tuple))

#define GMOCK_INTERNAL_HAS_NOEXCEPT(_Tuple) \
GMOCK_PP_HAS_COMMA( \
GMOCK_PP_FOR_EACH(GMOCK_INTERNAL_DETECT_NOEXCEPT, ~, _Tuple))
#define GMOCK_INTERNAL_GET_NOEXCEPT_SPEC(_Tuple) \
GMOCK_PP_FOR_EACH(GMOCK_INTERNAL_NOEXCEPT_SPEC_IF_NOEXCEPT, ~, _Tuple)

#define GMOCK_INTERNAL_NOEXCEPT_SPEC_IF_NOEXCEPT(_i, _, _elem) \
GMOCK_PP_IF( \
GMOCK_PP_HAS_COMMA(GMOCK_INTERNAL_DETECT_NOEXCEPT(_i, _, _elem)), _elem, )

#define GMOCK_INTERNAL_GET_CALLTYPE(_Tuple) \
GMOCK_PP_FOR_EACH(GMOCK_INTERNAL_GET_CALLTYPE_IMPL, ~, _Tuple)
Expand Down Expand Up @@ -180,7 +185,6 @@

#define GMOCK_INTERNAL_DETECT_FINAL_I_final ,

// TODO(iserna): Maybe noexcept should accept an argument here as well.
#define GMOCK_INTERNAL_DETECT_NOEXCEPT(_i, _, _elem) \
GMOCK_PP_CAT(GMOCK_INTERNAL_DETECT_NOEXCEPT_I_, _elem)

Expand Down
27 changes: 27 additions & 0 deletions googlemock/test/gmock-function-mocker_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@

#include <map>
#include <string>
#include <type_traits>
#include "gmock/gmock.h"
#include "gtest/gtest.h"

Expand Down Expand Up @@ -656,5 +657,31 @@ TEST(MockMethodMockFunctionTest, MockMethodSizeOverhead) {
EXPECT_EQ(sizeof(MockMethodSizes0), sizeof(MockMethodSizes4));
}

void hasTwoParams(int, int);
void MaybeThrows();
void DoesntThrow() noexcept;
struct MockMethodNoexceptSpecifier {
MOCK_METHOD(void, func1, (), (noexcept));
MOCK_METHOD(void, func2, (), (noexcept(true)));
MOCK_METHOD(void, func3, (), (noexcept(false)));
MOCK_METHOD(void, func4, (), (noexcept(noexcept(MaybeThrows()))));
MOCK_METHOD(void, func5, (), (noexcept(noexcept(DoesntThrow()))));
MOCK_METHOD(void, func6, (), (noexcept(noexcept(DoesntThrow())), const));
MOCK_METHOD(void, func7, (), (const, noexcept(noexcept(DoesntThrow()))));
// Put commas in the noexcept expression
MOCK_METHOD(void, func8, (), (noexcept(noexcept(hasTwoParams(1,2))), const));
};

TEST(MockMethodMockFunctionTest, NoexceptSpecifierPreserved) {
EXPECT_TRUE(noexcept(std::declval<MockMethodNoexceptSpecifier>().func1()));
Copy link

Choose a reason for hiding this comment

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

Is there a reason to use declval here? I'd simply define a MockMethodNoexceptSpecifier object.
In case the compiler warns that it's unused, it can be suppressed by cast to void.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had that previously, but the compiler would always say the result of the expression is noexcept(false).
https://travis-ci.org/google/googletest/jobs/594800623#L2313
My guess would be it has to do with the constructor not being marked noexcept.
But declval really is the "correct" solution as we just want to test the noexcept specifier for the function (and not the default constructor as well).

Another solution would be to use std::is_nothrow_invocable, but I don't think there's a technical reason to prefer one over another.

Copy link

Choose a reason for hiding this comment

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

My intention was to define one MockMethodNoexceptSpecifier object in the test, and use it in all expectations. Then it wouldn't matter if the constructor is noexcept.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's definitely another solution. Personally, I wouldn't like having to declare an instance of an object (which is a runtime construct) just so syntactically (since noexcept expressions are evaluated at compile-time) I can reason about the methods.
But I also can't argue it would be less code.
If this comes up in the "internal review" I'd be alright changing it.

Copy link

Choose a reason for hiding this comment

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

Instead of EXPECT_TRUE/FALSE, these could be tested at compile time with static_assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's all about tradeoffs if you want to use static_asserts in your test code to test some logic.
If you move it into a compile-time check, you know it'll always be true. The tests wouldn't compile otherwise. On the other hand, normally one test failing doesn't bar other tests from running and reporting their successes/failures. Since the noexcept spec wouldn't be absolutely fatal if it was wrong, I opted to implement them as runtime checks so they can report successes/failures just like other tests.

EXPECT_TRUE(noexcept(std::declval<MockMethodNoexceptSpecifier>().func2()));
EXPECT_FALSE(noexcept(std::declval<MockMethodNoexceptSpecifier>().func3()));
EXPECT_FALSE(noexcept(std::declval<MockMethodNoexceptSpecifier>().func4()));
EXPECT_TRUE(noexcept(std::declval<MockMethodNoexceptSpecifier>().func5()));
EXPECT_TRUE(noexcept(std::declval<MockMethodNoexceptSpecifier>().func6()));
EXPECT_TRUE(noexcept(std::declval<MockMethodNoexceptSpecifier>().func7()));
EXPECT_EQ(noexcept(std::declval<MockMethodNoexceptSpecifier>().func8()), noexcept(hasTwoParams(1,2)));
}
Copy link

Choose a reason for hiding this comment

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

I see no point in comparing with noexcept(1+1) - it simply evaluates to true. I suggest:

void Throws();
void DoesntThrow() noexcept;
struct MockMethodNoexceptSpecifier {
  MOCK_METHOD(void, func4, (), (noexcept(Throws())));
  MOCK_METHOD(void, func5, (), (noexcept(DoesntThrow())));
};
TEST(...) {
  EXPECT_FALSE(noexcept(mock.func4()));
  EXPECT_TRUE(noexcept(mock.func5()));
}


} // namespace gmock_function_mocker_test
} // namespace testing