diff --git a/google/cloud/spanner/value.cc b/google/cloud/spanner/value.cc index e2078998..4e3f79e7 100644 --- a/google/cloud/spanner/value.cc +++ b/google/cloud/spanner/value.cc @@ -287,6 +287,15 @@ StatusOr Value::GetValue(std::string const&, return pv.string_value(); } +StatusOr Value::GetValue(std::string const&, + google::protobuf::Value&& pv, + google::spanner::v1::Type const&) { + if (pv.kind_case() != google::protobuf::Value::kStringValue) { + return Status(StatusCode::kUnknown, "missing STRING"); + } + return std::move(*pv.mutable_string_value()); +} + StatusOr Value::GetValue(Bytes const&, google::protobuf::Value const& pv, google::spanner::v1::Type const&) { if (pv.kind_case() != google::protobuf::Value::kStringValue) { diff --git a/google/cloud/spanner/value.h b/google/cloud/spanner/value.h index 9fc9c0f4..0ac9f9b0 100644 --- a/google/cloud/spanner/value.h +++ b/google/cloud/spanner/value.h @@ -275,7 +275,7 @@ class Value { * @endcode */ template - StatusOr get() const { + StatusOr get() const& { // Ignores the name field because it is never set on the incoming `T`. if (!EqualTypeProtoIgnoringNames(type_, MakeTypeProto(T{}))) return Status(StatusCode::kUnknown, "wrong type"); @@ -286,6 +286,19 @@ class Value { return GetValue(T{}, value_, type_); } + /// @copydoc get() + template + StatusOr get() && { + // Ignores the name field because it is never set on the incoming `T`. + if (!EqualTypeProtoIgnoringNames(type_, MakeTypeProto(T{}))) + return Status(StatusCode::kUnknown, "wrong type"); + if (value_.kind_case() == google::protobuf::Value::kNullValue) { + if (is_optional::value) return T{}; + return Status(StatusCode::kUnknown, "null value"); + } + return GetValue(T{}, std::move(value_), type_); + } + /** * Allows Google Test to print internal debugging information when test * assertions fail. @@ -417,7 +430,7 @@ class Value { }; // Tag-dispatch overloads to extract a C++ value from a `Value` protobuf. The - // first argument type is the tag, the first argument value is ignored. + // first argument type is the tag, its value is ignored. static StatusOr GetValue(bool, google::protobuf::Value const&, google::spanner::v1::Type const&); static StatusOr GetValue(std::int64_t, @@ -428,48 +441,50 @@ class Value { static StatusOr GetValue(std::string const&, google::protobuf::Value const&, google::spanner::v1::Type const&); + static StatusOr GetValue(std::string const&, + google::protobuf::Value&&, + google::spanner::v1::Type const&); static StatusOr GetValue(Bytes const&, google::protobuf::Value const&, google::spanner::v1::Type const&); static StatusOr GetValue(Timestamp, google::protobuf::Value const&, google::spanner::v1::Type const&); static StatusOr GetValue(Date, google::protobuf::Value const&, google::spanner::v1::Type const&); - template - static StatusOr> GetValue(optional const&, - google::protobuf::Value const& pv, + template + static StatusOr> GetValue(optional const&, V&& pv, google::spanner::v1::Type const& pt) { if (pv.kind_case() == google::protobuf::Value::kNullValue) { return optional{}; } - auto value = GetValue(T{}, pv, pt); + auto value = GetValue(T{}, std::forward(pv), pt); if (!value) return std::move(value).status(); return optional{*std::move(value)}; } - template + template static StatusOr> GetValue( - std::vector const&, google::protobuf::Value const& pv, - google::spanner::v1::Type const& pt) { + std::vector const&, V&& pv, google::spanner::v1::Type const& pt) { if (pv.kind_case() != google::protobuf::Value::kListValue) { return Status(StatusCode::kUnknown, "missing ARRAY"); } std::vector v; - for (auto const& e : pv.list_value().values()) { - auto value = GetValue(T{}, e, pt.array_element_type()); + for (int i = 0; i < pv.list_value().values().size(); ++i) { + auto&& e = GetProtoListValueElement(std::forward(pv), i); + using ET = decltype(e); + auto value = GetValue(T{}, std::forward(e), pt.array_element_type()); if (!value) return std::move(value).status(); v.push_back(*std::move(value)); } return v; } - template + template static StatusOr> GetValue( - std::tuple const&, google::protobuf::Value const& pv, - google::spanner::v1::Type const& pt) { + std::tuple const&, V&& pv, google::spanner::v1::Type const& pt) { if (pv.kind_case() != google::protobuf::Value::kListValue) { return Status(StatusCode::kUnknown, "missing STRUCT"); } std::tuple tup; Status status; // OK - ExtractTupleValues f{status, 0, pv.list_value(), pt}; + ExtractTupleValues f{status, 0, std::forward(pv), pt}; internal::ForEach(tup, f); if (!status.ok()) return status; return tup; @@ -477,14 +492,17 @@ class Value { // A functor to be used with internal::ForEach (see below) to extract C++ // types from a ListValue proto and store then in a tuple. + template struct ExtractTupleValues { Status& status; int i; - google::protobuf::ListValue const& list_value; + V&& pv; google::spanner::v1::Type const& type; template void operator()(T& t) { - auto value = GetValue(T{}, list_value.values(i), type); + auto&& e = GetProtoListValueElement(std::forward(pv), i); + using ET = decltype(e); + auto value = GetValue(T{}, std::forward(e), type); ++i; if (!value) { status = std::move(value).status(); @@ -495,7 +513,9 @@ class Value { template void operator()(std::pair& p) { p.first = type.struct_type().fields(i).name(); - auto value = GetValue(T{}, list_value.values(i), type); + auto&& e = GetProtoListValueElement(std::forward(pv), i); + using ET = decltype(e); + auto value = GetValue(T{}, std::forward(e), type); ++i; if (!value) { status = std::move(value).status(); @@ -505,6 +525,19 @@ class Value { } }; + // Protocol buffers are not friendly to generic programming, because they use + // different syntax and different names for mutable and non-mutable + // functions. To make GetValue(vector, ...) (above) work, we need split + // the different protobuf syntaxes into overloaded functions. + static google::protobuf::Value const& GetProtoListValueElement( + google::protobuf::Value const& pv, int pos) { + return pv.list_value().values(pos); + } + static google::protobuf::Value&& GetProtoListValueElement( + google::protobuf::Value&& pv, int pos) { + return std::move(*pv.mutable_list_value()->mutable_values(pos)); + } + static bool EqualTypeProtoIgnoringNames(google::spanner::v1::Type const& a, google::spanner::v1::Type const& b); diff --git a/google/cloud/spanner/value_test.cc b/google/cloud/spanner/value_test.cc index 68f8cdb8..f14623c1 100644 --- a/google/cloud/spanner/value_test.cc +++ b/google/cloud/spanner/value_test.cc @@ -165,6 +165,107 @@ TEST(Value, BasicSemantics) { } } +// NOTE: This test relies on unspecified behavior about the moved-from state +// of std::string. Specifically, this test relies on the fact that "large" +// strings, when moved-from, end up empty. And we use this fact to verify that +// spanner::Value::get() correctly handles moves. If this test ever breaks +// on some platform, we could probably delete this, unless we can think of a +// better way to test move semantics. +TEST(Value, RvalueGetString) { + using Type = std::string; + Type const data = std::string(128, 'x'); + Value v(data); + + auto s = v.get(); + EXPECT_STATUS_OK(s); + EXPECT_EQ(data, *s); + + s = std::move(v).get(); + EXPECT_STATUS_OK(s); + EXPECT_EQ(data, *s); + + // NOLINTNEXTLINE(bugprone-use-after-move) + s = v.get(); + EXPECT_STATUS_OK(s); + EXPECT_EQ("", *s); +} + +// NOTE: This test relies on unspecified behavior about the moved-from state +// of std::string. Specifically, this test relies on the fact that "large" +// strings, when moved-from, end up empty. And we use this fact to verify that +// spanner::Value::get() correctly handles moves. If this test ever breaks +// on some platform, we could probably delete this, unless we can think of a +// better way to test move semantics. +TEST(Value, RvalueGetOptoinalString) { + using Type = optional; + Type const data = std::string(128, 'x'); + Value v(data); + + auto s = v.get(); + EXPECT_STATUS_OK(s); + EXPECT_EQ(*data, **s); + + s = std::move(v).get(); + EXPECT_STATUS_OK(s); + EXPECT_EQ(*data, **s); + + // NOLINTNEXTLINE(bugprone-use-after-move) + s = v.get(); + EXPECT_STATUS_OK(s); + EXPECT_EQ("", **s); +} + +// NOTE: This test relies on unspecified behavior about the moved-from state +// of std::string. Specifically, this test relies on the fact that "large" +// strings, when moved-from, end up empty. And we use this fact to verify that +// spanner::Value::get() correctly handles moves. If this test ever breaks +// on some platform, we could probably delete this, unless we can think of a +// better way to test move semantics. +TEST(Value, RvalueGetVectorString) { + using Type = std::vector; + Type const data(128, std::string(128, 'x')); + Value v(data); + + auto s = v.get(); + EXPECT_STATUS_OK(s); + EXPECT_EQ(data, *s); + + s = std::move(v).get(); + EXPECT_STATUS_OK(s); + EXPECT_EQ(data, *s); + + // NOLINTNEXTLINE(bugprone-use-after-move) + s = v.get(); + EXPECT_STATUS_OK(s); + EXPECT_EQ(Type(data.size(), ""), *s); +} + +// NOTE: This test relies on unspecified behavior about the moved-from state +// of std::string. Specifically, this test relies on the fact that "large" +// strings, when moved-from, end up empty. And we use this fact to verify that +// spanner::Value::get() correctly handles moves. If this test ever breaks +// on some platform, we could probably delete this, unless we can think of a +// better way to test move semantics. +TEST(Value, RvalueGetStructString) { + using Type = std::tuple, std::string>; + Type data{std::make_pair("name", std::string(128, 'x')), + std::string(128, 'x')}; + Value v(data); + + auto s = v.get(); + EXPECT_STATUS_OK(s); + EXPECT_EQ(data, *s); + + s = std::move(v).get(); + EXPECT_STATUS_OK(s); + EXPECT_EQ(data, *s); + + // NOLINTNEXTLINE(bugprone-use-after-move) + s = v.get(); + EXPECT_STATUS_OK(s); + EXPECT_EQ(Type({"name", ""}, ""), *s); +} + TEST(Value, DoubleNaN) { double const nan = std::nan("NaN"); Value v{nan};