diff --git a/src/google/protobuf/compiler/cpp/message.cc b/src/google/protobuf/compiler/cpp/message.cc index d5fb7afb93..765935ae8e 100644 --- a/src/google/protobuf/compiler/cpp/message.cc +++ b/src/google/protobuf/compiler/cpp/message.cc @@ -1160,19 +1160,18 @@ void MessageGenerator::GenerateMapEntryClassDefinition(io::Printer* p) { Formatter format(p); absl::flat_hash_map vars; CollectMapInfo(options_, descriptor_, &vars); - vars["lite"] = - HasDescriptorMethods(descriptor_->file(), options_) ? "" : "Lite"; + ABSL_CHECK(HasDescriptorMethods(descriptor_->file(), options_)); auto v = p->WithVars(std::move(vars)); // Templatize constexpr constructor as a workaround for a bug in gcc 12 // (warning in gcc 13). p->Emit(R"cc( class $classname$ final - : public ::$proto_ns$::internal::MapEntry$lite$< + : public ::$proto_ns$::internal::MapEntry< $classname$, $key_cpp$, $val_cpp$, ::$proto_ns$::internal::WireFormatLite::$key_wire_type$, ::$proto_ns$::internal::WireFormatLite::$val_wire_type$> { public: - using SuperType = ::$proto_ns$::internal::MapEntry$lite$< + using SuperType = ::$proto_ns$::internal::MapEntry< $classname$, $key_cpp$, $val_cpp$, ::$proto_ns$::internal::WireFormatLite::$key_wire_type$, ::$proto_ns$::internal::WireFormatLite::$val_wire_type$>; diff --git a/src/google/protobuf/map_entry.h b/src/google/protobuf/map_entry.h index 1bc1fc3c8b..dead40ac02 100644 --- a/src/google/protobuf/map_entry.h +++ b/src/google/protobuf/map_entry.h @@ -139,12 +139,6 @@ class MapEntry : public MapEntryBase { // accessors ====================================================== - inline const auto& key() const { - return KeyTypeHandler::GetExternalReference(key_); - } - inline const auto& value() const { - return ValueTypeHandler::DefaultIfNotInitialized(value_); - } inline auto* mutable_key() { _has_bits_[0] |= 0x00000001u; return KeyTypeHandler::EnsureMutable(&key_, GetArena()); @@ -153,7 +147,6 @@ class MapEntry : public MapEntryBase { _has_bits_[0] |= 0x00000002u; return ValueTypeHandler::EnsureMutable(&value_, GetArena()); } - // TODO: These methods currently differ in behavior from the ones // implemented via reflection. This means that a MapEntry does not behave the // same as an equivalent object made via DynamicMessage. @@ -185,24 +178,6 @@ class MapEntry : public MapEntryBase { return ptr; } - size_t ByteSizeLong() const final { - size_t size = 0; - size += kTagSize + static_cast(KeyTypeHandler::ByteSize(key())); - size += kTagSize + static_cast(ValueTypeHandler::ByteSize(value())); - _cached_size_.Set(ToCachedSize(size)); - return size; - } - - ::uint8_t* _InternalSerialize(::uint8_t* ptr, - io::EpsCopyOutputStream* stream) const final { - ptr = KeyTypeHandler::Write(kKeyFieldNumber, key(), ptr, stream); - return ValueTypeHandler::Write(kValueFieldNumber, value(), ptr, stream); - } - - bool IsInitialized() const final { - return ValueTypeHandler::IsInitialized(value_); - } - Message* New(Arena* arena) const final { return Arena::CreateMessage(arena); } diff --git a/src/google/protobuf/map_proto3_unittest.proto b/src/google/protobuf/map_proto3_unittest.proto index 1181b29988..29a3b367a2 100644 --- a/src/google/protobuf/map_proto3_unittest.proto +++ b/src/google/protobuf/map_proto3_unittest.proto @@ -21,3 +21,7 @@ message TestProto3BytesMap { map map_bytes = 1; map map_string = 2; } + +message TestI32StrMap { + map m_32_str = 1; +} diff --git a/src/google/protobuf/map_test.cc b/src/google/protobuf/map_test.cc index f5188f559f..7e9a80df42 100644 --- a/src/google/protobuf/map_test.cc +++ b/src/google/protobuf/map_test.cc @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -170,6 +171,48 @@ TEST(MapTest, CopyConstructMessagesWithArena) { EXPECT_EQ(map1["2"].GetArena(), &arena); } +TEST(MapTest, AlwaysSerializesBothEntries) { + for (const Message* prototype : + {static_cast( + &protobuf_unittest::TestI32StrMap::default_instance()), + static_cast( + &proto3_unittest::TestI32StrMap::default_instance())}) { + const FieldDescriptor* map_field = + prototype->GetDescriptor()->FindFieldByName("m_32_str"); + const FieldDescriptor* map_key = map_field->message_type()->map_key(); + const FieldDescriptor* map_value = map_field->message_type()->map_value(); + for (bool add_key : {true, false}) { + for (bool add_value : {true, false}) { + std::unique_ptr message(prototype->New()); + Message* entry_message = + message->GetReflection()->AddMessage(message.get(), map_field); + // Add the fields, but leave them as the default to make it easier to + // match. + if (add_key) { + entry_message->GetReflection()->SetInt32(entry_message, map_key, 0); + } + if (add_value) { + entry_message->GetReflection()->SetString(entry_message, map_value, + ""); + } + ASSERT_EQ(4, entry_message->ByteSizeLong()); + EXPECT_EQ(entry_message->SerializeAsString(), + std::string({ + '\010', '\0', // key, VARINT, value=0 + '\022', '\0', // value, LEN, size=0 + })); + ASSERT_EQ(6, message->ByteSizeLong()); + EXPECT_EQ(message->SerializeAsString(), + std::string({ + '\012', '\04', // field=1, LEN, size=4 + '\010', '\0', // key, VARINT, value=0 + '\022', '\0', // value, LEN, size=0 + })); + } + } + } +} + TEST(MapTest, LoadFactorCalculationWorks) { // Three stages: // - empty diff --git a/src/google/protobuf/map_type_handler.h b/src/google/protobuf/map_type_handler.h index 7475ab04a3..7429dae7de 100644 --- a/src/google/protobuf/map_type_handler.h +++ b/src/google/protobuf/map_type_handler.h @@ -93,16 +93,10 @@ class MapTypeHandler { uint8_t* ptr, io::EpsCopyOutputStream* stream); // Functions to manipulate data on memory. ======================== - static inline const Type& GetExternalReference(const Type* value); static inline void DeleteNoArena(const Type* x); static constexpr TypeOnMemory Constinit(); static inline Type* EnsureMutable(Type** value, Arena* arena); - // Return default instance if value is not initialized when calling const - // reference accessor. - static inline const Type& DefaultIfNotInitialized(const Type* value); - // Check if all required fields have values set. - static inline bool IsInitialized(Type* value); }; #define MAP_HANDLER(FieldType) \ @@ -126,12 +120,7 @@ class MapTypeHandler { static inline uint8_t* Write(int field, const MapEntryAccessorType& value, \ uint8_t* ptr, \ io::EpsCopyOutputStream* stream); \ - static inline const MapEntryAccessorType& GetExternalReference( \ - const TypeOnMemory& value); \ static inline void DeleteNoArena(const TypeOnMemory& x); \ - static inline const MapEntryAccessorType& DefaultIfNotInitialized( \ - const TypeOnMemory& value); \ - static inline bool IsInitialized(const TypeOnMemory& value); \ static void DeleteNoArena(TypeOnMemory& value); \ static constexpr TypeOnMemory Constinit(); \ static inline MapEntryAccessorType* EnsureMutable(TypeOnMemory* value, \ @@ -420,13 +409,6 @@ READ_METHOD(BOOL) // Definition for message handler -template -inline const Type& -MapTypeHandler::GetExternalReference( - const Type* value) { - return *value; -} - template void MapTypeHandler::DeleteNoArena( const Type* ptr) { @@ -448,29 +430,9 @@ inline Type* MapTypeHandler::EnsureMutable( return *value; } -template -inline const Type& -MapTypeHandler::DefaultIfNotInitialized( - const Type* value) { - return value != nullptr ? *value : *Type::internal_default_instance(); -} - -template -inline bool MapTypeHandler::IsInitialized( - Type* value) { - return value ? value->IsInitialized() : false; -} - // Definition for string/bytes handler #define STRING_OR_BYTES_HANDLER_FUNCTIONS(FieldType) \ - template \ - inline const typename MapTypeHandler::MapEntryAccessorType& \ - MapTypeHandler::GetExternalReference(const TypeOnMemory& value) { \ - return value.Get(); \ - } \ template \ void MapTypeHandler::DeleteNoArena( \ TypeOnMemory& value) { \ @@ -479,7 +441,7 @@ inline bool MapTypeHandler::IsInitialized( template \ constexpr auto \ MapTypeHandler::Constinit() \ - ->TypeOnMemory { \ + -> TypeOnMemory { \ return TypeOnMemory(&internal::fixed_address_empty_string, \ ConstantInitialized{}); \ } \ @@ -489,32 +451,12 @@ inline bool MapTypeHandler::IsInitialized( MapTypeHandler::EnsureMutable( \ TypeOnMemory* value, Arena* arena) { \ return value->Mutable(arena); \ - } \ - template \ - inline const typename MapTypeHandler::MapEntryAccessorType& \ - MapTypeHandler::DefaultIfNotInitialized(const TypeOnMemory& value) { \ - return value.Get(); \ - } \ - template \ - inline bool \ - MapTypeHandler::IsInitialized( \ - const TypeOnMemory& /* value */) { \ - return true; \ } STRING_OR_BYTES_HANDLER_FUNCTIONS(STRING) STRING_OR_BYTES_HANDLER_FUNCTIONS(BYTES) #undef STRING_OR_BYTES_HANDLER_FUNCTIONS #define PRIMITIVE_HANDLER_FUNCTIONS(FieldType) \ - template \ - inline const typename MapTypeHandler::MapEntryAccessorType& \ - MapTypeHandler::GetExternalReference(const TypeOnMemory& value) { \ - return value; \ - } \ template \ inline void MapTypeHandler::DeleteNoArena(TypeOnMemory& /* x */) {} \ @@ -530,19 +472,6 @@ STRING_OR_BYTES_HANDLER_FUNCTIONS(BYTES) MapTypeHandler::EnsureMutable( \ TypeOnMemory* value, Arena* /* arena */) { \ return value; \ - } \ - template \ - inline const typename MapTypeHandler::MapEntryAccessorType& \ - MapTypeHandler::DefaultIfNotInitialized(const TypeOnMemory& value) { \ - return value; \ - } \ - template \ - inline bool \ - MapTypeHandler::IsInitialized( \ - const TypeOnMemory& /* value */) { \ - return true; \ } PRIMITIVE_HANDLER_FUNCTIONS(INT64) PRIMITIVE_HANDLER_FUNCTIONS(UINT64) diff --git a/src/google/protobuf/map_unittest.proto b/src/google/protobuf/map_unittest.proto index 11be80836e..5a648d202f 100644 --- a/src/google/protobuf/map_unittest.proto +++ b/src/google/protobuf/map_unittest.proto @@ -101,3 +101,7 @@ message MessageContainingMapCalledEntry { message TestRecursiveMapMessage { map a = 1; } + +message TestI32StrMap { + map m_32_str = 1; +} diff --git a/src/google/protobuf/reflection_ops.cc b/src/google/protobuf/reflection_ops.cc index 7e125e2656..233d25847c 100644 --- a/src/google/protobuf/reflection_ops.cc +++ b/src/google/protobuf/reflection_ops.cc @@ -262,7 +262,13 @@ bool ReflectionOps::IsInitialized(const Message& message) { std::vector fields; // Should be safe to skip stripped fields because required fields are not // stripped. - reflection->ListFields(message, &fields); + if (descriptor->options().map_entry()) { + // MapEntry objects always check the value regardless of has bit. + // We don't need to bother with the key. + fields = {descriptor->map_value()}; + } else { + reflection->ListFields(message, &fields); + } for (const FieldDescriptor* field : fields) { if (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) {