Skip to content

Commit 4224c77

Browse files
jacobsacopybara-github
jacobsa
authored and
copybara-github
committed
gmock-actions: simplify Return and add better documentation.
Better document requirements, API decisions, and historical accidents. Make an implicit conversion easier and in a more appropriate place, and ease the burden of some assertions in the conversion operator. Stop using the legacy ActionInterface style for defining the action. PiperOrigin-RevId: 447894892 Change-Id: I179e23ec2abdd9bf05c204ab18dbb492f1372e8e
1 parent 6386897 commit 4224c77

File tree

3 files changed

+192
-93
lines changed

3 files changed

+192
-93
lines changed

‎googlemock/include/gmock/gmock-actions.h‎

Lines changed: 158 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -866,93 +866,155 @@ struct ByMoveWrapper {
866866
T payload;
867867
};
868868

869-
// Implements the polymorphic Return(x) action, which can be used in
870-
// any function that returns the type of x, regardless of the argument
871-
// types.
872-
//
873-
// Note: The value passed into Return must be converted into
874-
// Function<F>::Result when this action is cast to Action<F> rather than
875-
// when that action is performed. This is important in scenarios like
876-
//
877-
// MOCK_METHOD1(Method, T(U));
878-
// ...
879-
// {
880-
// Foo foo;
881-
// X x(&foo);
882-
// EXPECT_CALL(mock, Method(_)).WillOnce(Return(x));
883-
// }
884-
//
885-
// In the example above the variable x holds reference to foo which leaves
886-
// scope and gets destroyed. If copying X just copies a reference to foo,
887-
// that copy will be left with a hanging reference. If conversion to T
888-
// makes a copy of foo, the above code is safe. To support that scenario, we
889-
// need to make sure that the type conversion happens inside the EXPECT_CALL
890-
// statement, and conversion of the result of Return to Action<T(U)> is a
891-
// good place for that.
892-
//
893-
// The real life example of the above scenario happens when an invocation
894-
// of gtl::Container() is passed into Return.
895-
//
869+
// The general implementation of Return(R). Specializations follow below.
896870
template <typename R>
897871
class ReturnAction final {
898872
public:
899-
// Constructs a ReturnAction object from the value to be returned.
900-
// 'value' is passed by value instead of by const reference in order
901-
// to allow Return("string literal") to compile.
902873
explicit ReturnAction(R value) : value_(std::move(value)) {}
903874

904-
// This template type conversion operator allows Return(x) to be
905-
// used in ANY function that returns x's type.
906-
template <typename F>
907-
operator Action<F>() const { // NOLINT
908-
// Assert statement belongs here because this is the best place to verify
909-
// conditions on F. It produces the clearest error messages
910-
// in most compilers.
911-
// Impl really belongs in this scope as a local class but can't
912-
// because MSVC produces duplicate symbols in different translation units
913-
// in this case. Until MS fixes that bug we put Impl into the class scope
914-
// and put the typedef both here (for use in assert statement) and
915-
// in the Impl class. But both definitions must be the same.
916-
typedef typename Function<F>::Result Result;
917-
static_assert(!std::is_reference<Result>::value,
875+
// Support conversion to function types with compatible return types. See the
876+
// documentation on Return for the definition of compatible.
877+
template <typename U, typename... Args>
878+
operator Action<U(Args...)>() const { // NOLINT
879+
// Check our requirements on the return type.
880+
static_assert(!std::is_reference<U>::value,
918881
"use ReturnRef instead of Return to return a reference");
919-
static_assert(!std::is_void<Result>::value,
882+
883+
static_assert(!std::is_void<U>::value,
920884
"Can't use Return() on an action expected to return `void`.");
921-
return Action<F>(new Impl<F>(value_));
885+
886+
return Impl<U>(value_);
922887
}
923888

924889
private:
925-
// Implements the Return(x) action for a particular function type F.
926-
template <typename F>
927-
class Impl : public ActionInterface<F> {
890+
// Implements the Return(x) action for a mock function that returns type U.
891+
template <typename U>
892+
class Impl final {
928893
public:
929-
typedef typename Function<F>::Result Result;
930-
typedef typename Function<F>::ArgumentTuple ArgumentTuple;
894+
explicit Impl(const R& input_value) : state_(new State(input_value)) {}
931895

932-
explicit Impl(const R& value)
933-
: value_before_cast_(value),
934-
// Make an implicit conversion to Result before initializing the
935-
// Result object we store, avoiding calling any explicit constructor
936-
// of Result from R.
937-
//
938-
// This simulates the language rules: a function with return type
939-
// Result that does `return R()` requires R to be implicitly
940-
// convertible to Result, and uses that path for the conversion, even
941-
// if Result has an explicit constructor from R.
942-
value_(ImplicitCast_<Result>(value_before_cast_)) {}
943-
944-
Result Perform(const ArgumentTuple&) override { return value_; }
896+
U operator()() const { return state_->value; }
945897

946898
private:
947-
static_assert(!std::is_reference<Result>::value,
948-
"Result cannot be a reference type");
949-
// We save the value before casting just in case it is being cast to a
950-
// wrapper type.
951-
R value_before_cast_;
952-
Result value_;
953-
954-
Impl(const Impl&) = delete;
955-
Impl& operator=(const Impl&) = delete;
899+
// We put our state on the heap so that the compiler-generated copy/move
900+
// constructors work correctly even when U is a reference-like type. This is
901+
// necessary only because we eagerly create State::value (see the note on
902+
// that symbol for details). If we instead had only the input value as a
903+
// member then the default constructors would work fine.
904+
//
905+
// For example, when R is std::string and U is std::string_view, value is a
906+
// reference to the string backed by input_value. The copy constructor would
907+
// copy both, so that we wind up with a new input_value object (with the
908+
// same contents) and a reference to the *old* input_value object rather
909+
// than the new one.
910+
struct State {
911+
explicit State(const R& input_value_in)
912+
: input_value(input_value_in),
913+
// Make an implicit conversion to Result before initializing the U
914+
// object we store, avoiding calling any explicit constructor of U
915+
// from R.
916+
//
917+
// This simulates the language rules: a function with return type U
918+
// that does `return R()` requires R to be implicitly convertible to
919+
// U, and uses that path for the conversion, even U Result has an
920+
// explicit constructor from R.
921+
//
922+
// We provide non-const access to input_value to the conversion
923+
// code. It's not clear whether this makes semantic sense -- what
924+
// would it mean for the conversion to modify the input value? This
925+
// appears to be an accident of history:
926+
//
927+
// 1. Before the first public commit the input value was simply an
928+
// object of type R embedded directly in the Impl object. The
929+
// result value wasn't yet eagerly created, and the Impl class's
930+
// Perform method was const, so the implicit conversion when it
931+
// returned the value was from const R&.
932+
//
933+
// 2. Google changelist 6490411 changed ActionInterface::Perform to
934+
// be non-const, citing the fact that an action can have side
935+
// effects and be stateful. Impl::Perform was updated like all
936+
// other actions, probably without consideration of the fact
937+
// that side effects and statefulness don't make sense for
938+
// Return. From this point on the conversion had non-const
939+
// access to the input value.
940+
//
941+
value(ImplicitCast_<U>(input_value)) {}
942+
943+
// A copy of the value originally provided by the user. We retain this in
944+
// addition to the value of the mock function's result type below in case
945+
// the latter is a reference-like type. See the std::string_view example
946+
// in the documentation on Return.
947+
R input_value;
948+
949+
// The value we actually return, as the type returned by the mock function
950+
// itself.
951+
//
952+
// We eagerly initialize this here, rather than lazily doing the implicit
953+
// conversion automatically each time Perform is called, for historical
954+
// reasons: in 2009-11, commit a070cbd91c (Google changelist 13540126)
955+
// made the Action<U()> conversion operator eagerly convert the R value to
956+
// U, but without keeping the R alive. This broke the use case discussed
957+
// in the documentation for Return, making reference-like types such as
958+
// std::string_view not safe to use as U where the input type R is a
959+
// value-like type such as std::string.
960+
//
961+
// The example the commit gave was not very clear, nor was the issue
962+
// thread (https://github.com/google/googlemock/issues/86), but it seems
963+
// the worry was about reference-like input types R that flatten to a
964+
// value-like type U when being implicitly converted. An example of this
965+
// is std::vector<bool>::reference, which is often a proxy type with an
966+
// reference to the underlying vector:
967+
//
968+
// // Helper method: have the mock function return bools according
969+
// // to the supplied script.
970+
// void SetActions(MockFunction<bool(size_t)>& mock,
971+
// const std::vector<bool>& script) {
972+
// for (size_t i = 0; i < script.size(); ++i) {
973+
// EXPECT_CALL(mock, Call(i)).WillOnce(Return(script[i]));
974+
// }
975+
// }
976+
//
977+
// TEST(Foo, Bar) {
978+
// // Set actions using a temporary vector, whose operator[]
979+
// // returns proxy objects that references that will be
980+
// // dangling once the call to SetActions finishes and the
981+
// // vector is destroyed.
982+
// MockFunction<bool(size_t)> mock;
983+
// SetActions(mock, {false, true});
984+
//
985+
// EXPECT_FALSE(mock.AsStdFunction()(0));
986+
// EXPECT_TRUE(mock.AsStdFunction()(1));
987+
// }
988+
//
989+
// This eager conversion helps with a simple case like this, but doesn't
990+
// fully make these types work in general. For example the following still
991+
// uses a dangling reference:
992+
//
993+
// TEST(Foo, Baz) {
994+
// MockFunction<std::vector<std::string>()> mock;
995+
//
996+
// // Return the same vector twice, and then the empty vector
997+
// // thereafter.
998+
// auto action = Return(std::initializer_list<std::string>{
999+
// "taco", "burrito",
1000+
// });
1001+
//
1002+
// EXPECT_CALL(mock, Call)
1003+
// .WillOnce(action)
1004+
// .WillOnce(action)
1005+
// .WillRepeatedly(Return(std::vector<std::string>{}));
1006+
//
1007+
// EXPECT_THAT(mock.AsStdFunction()(),
1008+
// ElementsAre("taco", "burrito"));
1009+
// EXPECT_THAT(mock.AsStdFunction()(),
1010+
// ElementsAre("taco", "burrito"));
1011+
// EXPECT_THAT(mock.AsStdFunction()(), IsEmpty());
1012+
// }
1013+
//
1014+
U value;
1015+
};
1016+
1017+
const std::shared_ptr<State> state_;
9561018
};
9571019

9581020
R value_;
@@ -1693,9 +1755,29 @@ internal::WithArgsAction<typename std::decay<InnerAction>::type> WithoutArgs(
16931755
return {std::forward<InnerAction>(action)};
16941756
}
16951757

1696-
// Creates an action that returns 'value'. 'value' is passed by value
1697-
// instead of const reference - otherwise Return("string literal")
1698-
// will trigger a compiler error about using array as initializer.
1758+
// Creates an action that returns a value.
1759+
//
1760+
// R must be copy-constructible. The returned type can be used as an
1761+
// Action<U(Args...)> for any type U where all of the following are true:
1762+
//
1763+
// * U is not void.
1764+
// * U is not a reference type. (Use ReturnRef instead.)
1765+
// * U is copy-constructible.
1766+
// * R& is convertible to U.
1767+
//
1768+
// The Action<U(Args)...> object contains the R value from which the U return
1769+
// value is constructed (a copy of the argument to Return). This means that the
1770+
// R value will survive at least until the mock object's expectations are
1771+
// cleared or the mock object is destroyed, meaning that U can be a
1772+
// reference-like type such as std::string_view:
1773+
//
1774+
// // The mock function returns a view of a copy of the string fed to
1775+
// // Return. The view is valid even after the action is performed.
1776+
// MockFunction<std::string_view()> mock;
1777+
// EXPECT_CALL(mock, Call).WillOnce(Return(std::string("taco")));
1778+
// const std::string_view result = mock.AsStdFunction()();
1779+
// EXPECT_EQ("taco", result);
1780+
//
16991781
template <typename R>
17001782
internal::ReturnAction<R> Return(R value) {
17011783
return internal::ReturnAction<R>(std::move(value));

‎googlemock/test/gmock-actions_test.cc‎

Lines changed: 28 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
#include <memory>
5252
#include <string>
5353
#include <type_traits>
54+
#include <vector>
5455

5556
#include "gmock/gmock.h"
5657
#include "gmock/internal/gmock-port.h"
@@ -647,24 +648,34 @@ TEST(ReturnTest, AcceptsStringLiteral) {
647648
EXPECT_EQ("world", a2.Perform(std::make_tuple()));
648649
}
649650

650-
// Test struct which wraps a vector of integers. Used in
651-
// 'SupportsWrapperReturnType' test.
652-
struct IntegerVectorWrapper {
653-
std::vector<int>* v;
654-
IntegerVectorWrapper(std::vector<int>& _v) : v(&_v) {} // NOLINT
655-
};
651+
// Return(x) should work fine when the mock function's return type is a
652+
// reference-like wrapper for decltype(x), as when x is a std::string and the
653+
// mock function returns std::string_view.
654+
TEST(ReturnTest, SupportsReferenceLikeReturnType) {
655+
// A reference wrapper for std::vector<int>, implicitly convertible from it.
656+
struct Result {
657+
std::vector<int>* v;
658+
Result(std::vector<int>& v) : v(&v) {} // NOLINT
659+
};
660+
661+
// Set up an action for a mock function that returns the reference wrapper
662+
// type, initializing it with an actual vector.
663+
//
664+
// The returned wrapper should be initialized with a copy of that vector
665+
// that's embedded within the action itself (which should stay alive as long
666+
// as the mock object is alive), rather than e.g. a reference to the temporary
667+
// we feed to Return. This should work fine both for WillOnce and
668+
// WillRepeatedly.
669+
MockFunction<Result()> mock;
670+
EXPECT_CALL(mock, Call)
671+
.WillOnce(Return(std::vector<int>{17, 19, 23}))
672+
.WillRepeatedly(Return(std::vector<int>{29, 31, 37}));
673+
674+
EXPECT_THAT(mock.AsStdFunction()(),
675+
Field(&Result::v, Pointee(ElementsAre(17, 19, 23))));
656676

657-
// Tests that Return() works when return type is a wrapper type.
658-
TEST(ReturnTest, SupportsWrapperReturnType) {
659-
// Initialize vector of integers.
660-
std::vector<int> v;
661-
for (int i = 0; i < 5; ++i) v.push_back(i);
662-
663-
// Return() called with 'v' as argument. The Action will return the same data
664-
// as 'v' (copy) but it will be wrapped in an IntegerVectorWrapper.
665-
Action<IntegerVectorWrapper()> a = Return(v);
666-
const std::vector<int>& result = *(a.Perform(std::make_tuple()).v);
667-
EXPECT_THAT(result, ::testing::ElementsAre(0, 1, 2, 3, 4));
677+
EXPECT_THAT(mock.AsStdFunction()(),
678+
Field(&Result::v, Pointee(ElementsAre(29, 31, 37))));
668679
}
669680

670681
TEST(ReturnTest, PrefersConversionOperator) {

‎googlemock/test/gmock-function-mocker_test.cc‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,12 @@
2727
// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
2828
// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
2929

30+
// Silence C4503 (decorated name length exceeded) for MSVC.
31+
#ifdef _MSC_VER
32+
#pragma warning(push)
33+
#pragma warning(disable : 4503)
34+
#endif
35+
3036
// Google Mock - a framework for writing C++ mock classes.
3137
//
3238
// This file tests the function mocker classes.

0 commit comments

Comments
 (0)