Skip to content

Commit 2b45018

Browse files
protobuf-github-botcopybara-github
protobuf-github-bot
authored and
copybara-github
committed
Fix some of the reflection based algorithms to handle Map entries properly.
They should ignore presence and always process key and value. Remove codegen for methods in MapEntry and use the ones from Message. Delete dead code in MapTypeHandler. PiperOrigin-RevId: 588826780
1 parent 5341189 commit 2b45018

File tree

7 files changed

+62
-102
lines changed

7 files changed

+62
-102
lines changed

‎src/google/protobuf/compiler/cpp/message.cc‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1160,19 +1160,18 @@ void MessageGenerator::GenerateMapEntryClassDefinition(io::Printer* p) {
11601160
Formatter format(p);
11611161
absl::flat_hash_map<absl::string_view, std::string> vars;
11621162
CollectMapInfo(options_, descriptor_, &vars);
1163-
vars["lite"] =
1164-
HasDescriptorMethods(descriptor_->file(), options_) ? "" : "Lite";
1163+
ABSL_CHECK(HasDescriptorMethods(descriptor_->file(), options_));
11651164
auto v = p->WithVars(std::move(vars));
11661165
// Templatize constexpr constructor as a workaround for a bug in gcc 12
11671166
// (warning in gcc 13).
11681167
p->Emit(R"cc(
11691168
class $classname$ final
1170-
: public ::$proto_ns$::internal::MapEntry$lite$<
1169+
: public ::$proto_ns$::internal::MapEntry<
11711170
$classname$, $key_cpp$, $val_cpp$,
11721171
::$proto_ns$::internal::WireFormatLite::$key_wire_type$,
11731172
::$proto_ns$::internal::WireFormatLite::$val_wire_type$> {
11741173
public:
1175-
using SuperType = ::$proto_ns$::internal::MapEntry$lite$<
1174+
using SuperType = ::$proto_ns$::internal::MapEntry<
11761175
$classname$, $key_cpp$, $val_cpp$,
11771176
::$proto_ns$::internal::WireFormatLite::$key_wire_type$,
11781177
::$proto_ns$::internal::WireFormatLite::$val_wire_type$>;

‎src/google/protobuf/map_entry.h‎

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -139,12 +139,6 @@ class MapEntry : public MapEntryBase {
139139

140140
// accessors ======================================================
141141

142-
inline const auto& key() const {
143-
return KeyTypeHandler::GetExternalReference(key_);
144-
}
145-
inline const auto& value() const {
146-
return ValueTypeHandler::DefaultIfNotInitialized(value_);
147-
}
148142
inline auto* mutable_key() {
149143
_has_bits_[0] |= 0x00000001u;
150144
return KeyTypeHandler::EnsureMutable(&key_, GetArena());
@@ -153,7 +147,6 @@ class MapEntry : public MapEntryBase {
153147
_has_bits_[0] |= 0x00000002u;
154148
return ValueTypeHandler::EnsureMutable(&value_, GetArena());
155149
}
156-
157150
// TODO: These methods currently differ in behavior from the ones
158151
// implemented via reflection. This means that a MapEntry does not behave the
159152
// same as an equivalent object made via DynamicMessage.
@@ -185,24 +178,6 @@ class MapEntry : public MapEntryBase {
185178
return ptr;
186179
}
187180

188-
size_t ByteSizeLong() const final {
189-
size_t size = 0;
190-
size += kTagSize + static_cast<size_t>(KeyTypeHandler::ByteSize(key()));
191-
size += kTagSize + static_cast<size_t>(ValueTypeHandler::ByteSize(value()));
192-
_cached_size_.Set(ToCachedSize(size));
193-
return size;
194-
}
195-
196-
::uint8_t* _InternalSerialize(::uint8_t* ptr,
197-
io::EpsCopyOutputStream* stream) const final {
198-
ptr = KeyTypeHandler::Write(kKeyFieldNumber, key(), ptr, stream);
199-
return ValueTypeHandler::Write(kValueFieldNumber, value(), ptr, stream);
200-
}
201-
202-
bool IsInitialized() const final {
203-
return ValueTypeHandler::IsInitialized(value_);
204-
}
205-
206181
Message* New(Arena* arena) const final {
207182
return Arena::CreateMessage<Derived>(arena);
208183
}

‎src/google/protobuf/map_proto3_unittest.proto‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,3 +21,7 @@ message TestProto3BytesMap {
2121
map<int32, bytes> map_bytes = 1;
2222
map<int32, string> map_string = 2;
2323
}
24+
25+
message TestI32StrMap {
26+
map<int32, string> m_32_str = 1;
27+
}

‎src/google/protobuf/map_test.cc‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include <array>
1111
#include <cstddef>
1212
#include <cstdint>
13+
#include <memory>
1314
#include <string>
1415
#include <type_traits>
1516
#include <utility>
@@ -170,6 +171,48 @@ TEST(MapTest, CopyConstructMessagesWithArena) {
170171
EXPECT_EQ(map1["2"].GetArena(), &arena);
171172
}
172173

174+
TEST(MapTest, AlwaysSerializesBothEntries) {
175+
for (const Message* prototype :
176+
{static_cast<const Message*>(
177+
&protobuf_unittest::TestI32StrMap::default_instance()),
178+
static_cast<const Message*>(
179+
&proto3_unittest::TestI32StrMap::default_instance())}) {
180+
const FieldDescriptor* map_field =
181+
prototype->GetDescriptor()->FindFieldByName("m_32_str");
182+
const FieldDescriptor* map_key = map_field->message_type()->map_key();
183+
const FieldDescriptor* map_value = map_field->message_type()->map_value();
184+
for (bool add_key : {true, false}) {
185+
for (bool add_value : {true, false}) {
186+
std::unique_ptr<Message> message(prototype->New());
187+
Message* entry_message =
188+
message->GetReflection()->AddMessage(message.get(), map_field);
189+
// Add the fields, but leave them as the default to make it easier to
190+
// match.
191+
if (add_key) {
192+
entry_message->GetReflection()->SetInt32(entry_message, map_key, 0);
193+
}
194+
if (add_value) {
195+
entry_message->GetReflection()->SetString(entry_message, map_value,
196+
"");
197+
}
198+
ASSERT_EQ(4, entry_message->ByteSizeLong());
199+
EXPECT_EQ(entry_message->SerializeAsString(),
200+
std::string({
201+
'\010', '\0', // key, VARINT, value=0
202+
'\022', '\0', // value, LEN, size=0
203+
}));
204+
ASSERT_EQ(6, message->ByteSizeLong());
205+
EXPECT_EQ(message->SerializeAsString(),
206+
std::string({
207+
'\012', '\04', // field=1, LEN, size=4
208+
'\010', '\0', // key, VARINT, value=0
209+
'\022', '\0', // value, LEN, size=0
210+
}));
211+
}
212+
}
213+
}
214+
}
215+
173216
TEST(MapTest, LoadFactorCalculationWorks) {
174217
// Three stages:
175218
// - empty

‎src/google/protobuf/map_type_handler.h‎

Lines changed: 1 addition & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -93,16 +93,10 @@ class MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type> {
9393
uint8_t* ptr, io::EpsCopyOutputStream* stream);
9494

9595
// Functions to manipulate data on memory. ========================
96-
static inline const Type& GetExternalReference(const Type* value);
9796
static inline void DeleteNoArena(const Type* x);
9897
static constexpr TypeOnMemory Constinit();
9998

10099
static inline Type* EnsureMutable(Type** value, Arena* arena);
101-
// Return default instance if value is not initialized when calling const
102-
// reference accessor.
103-
static inline const Type& DefaultIfNotInitialized(const Type* value);
104-
// Check if all required fields have values set.
105-
static inline bool IsInitialized(Type* value);
106100
};
107101

108102
#define MAP_HANDLER(FieldType) \
@@ -126,12 +120,7 @@ class MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type> {
126120
static inline uint8_t* Write(int field, const MapEntryAccessorType& value, \
127121
uint8_t* ptr, \
128122
io::EpsCopyOutputStream* stream); \
129-
static inline const MapEntryAccessorType& GetExternalReference( \
130-
const TypeOnMemory& value); \
131123
static inline void DeleteNoArena(const TypeOnMemory& x); \
132-
static inline const MapEntryAccessorType& DefaultIfNotInitialized( \
133-
const TypeOnMemory& value); \
134-
static inline bool IsInitialized(const TypeOnMemory& value); \
135124
static void DeleteNoArena(TypeOnMemory& value); \
136125
static constexpr TypeOnMemory Constinit(); \
137126
static inline MapEntryAccessorType* EnsureMutable(TypeOnMemory* value, \
@@ -420,13 +409,6 @@ READ_METHOD(BOOL)
420409

421410
// Definition for message handler
422411

423-
template <typename Type>
424-
inline const Type&
425-
MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::GetExternalReference(
426-
const Type* value) {
427-
return *value;
428-
}
429-
430412
template <typename Type>
431413
void MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::DeleteNoArena(
432414
const Type* ptr) {
@@ -448,29 +430,9 @@ inline Type* MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::EnsureMutable(
448430
return *value;
449431
}
450432

451-
template <typename Type>
452-
inline const Type&
453-
MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::DefaultIfNotInitialized(
454-
const Type* value) {
455-
return value != nullptr ? *value : *Type::internal_default_instance();
456-
}
457-
458-
template <typename Type>
459-
inline bool MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::IsInitialized(
460-
Type* value) {
461-
return value ? value->IsInitialized() : false;
462-
}
463-
464433
// Definition for string/bytes handler
465434

466435
#define STRING_OR_BYTES_HANDLER_FUNCTIONS(FieldType) \
467-
template <typename Type> \
468-
inline const typename MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
469-
Type>::MapEntryAccessorType& \
470-
MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
471-
Type>::GetExternalReference(const TypeOnMemory& value) { \
472-
return value.Get(); \
473-
} \
474436
template <typename Type> \
475437
void MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::DeleteNoArena( \
476438
TypeOnMemory& value) { \
@@ -479,7 +441,7 @@ inline bool MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::IsInitialized(
479441
template <typename Type> \
480442
constexpr auto \
481443
MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::Constinit() \
482-
->TypeOnMemory { \
444+
-> TypeOnMemory { \
483445
return TypeOnMemory(&internal::fixed_address_empty_string, \
484446
ConstantInitialized{}); \
485447
} \
@@ -489,32 +451,12 @@ inline bool MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::IsInitialized(
489451
MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::EnsureMutable( \
490452
TypeOnMemory* value, Arena* arena) { \
491453
return value->Mutable(arena); \
492-
} \
493-
template <typename Type> \
494-
inline const typename MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
495-
Type>::MapEntryAccessorType& \
496-
MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
497-
Type>::DefaultIfNotInitialized(const TypeOnMemory& value) { \
498-
return value.Get(); \
499-
} \
500-
template <typename Type> \
501-
inline bool \
502-
MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::IsInitialized( \
503-
const TypeOnMemory& /* value */) { \
504-
return true; \
505454
}
506455
STRING_OR_BYTES_HANDLER_FUNCTIONS(STRING)
507456
STRING_OR_BYTES_HANDLER_FUNCTIONS(BYTES)
508457
#undef STRING_OR_BYTES_HANDLER_FUNCTIONS
509458

510459
#define PRIMITIVE_HANDLER_FUNCTIONS(FieldType) \
511-
template <typename Type> \
512-
inline const typename MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
513-
Type>::MapEntryAccessorType& \
514-
MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
515-
Type>::GetExternalReference(const TypeOnMemory& value) { \
516-
return value; \
517-
} \
518460
template <typename Type> \
519461
inline void MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
520462
Type>::DeleteNoArena(TypeOnMemory& /* x */) {} \
@@ -530,19 +472,6 @@ STRING_OR_BYTES_HANDLER_FUNCTIONS(BYTES)
530472
MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::EnsureMutable( \
531473
TypeOnMemory* value, Arena* /* arena */) { \
532474
return value; \
533-
} \
534-
template <typename Type> \
535-
inline const typename MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
536-
Type>::MapEntryAccessorType& \
537-
MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
538-
Type>::DefaultIfNotInitialized(const TypeOnMemory& value) { \
539-
return value; \
540-
} \
541-
template <typename Type> \
542-
inline bool \
543-
MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::IsInitialized( \
544-
const TypeOnMemory& /* value */) { \
545-
return true; \
546475
}
547476
PRIMITIVE_HANDLER_FUNCTIONS(INT64)
548477
PRIMITIVE_HANDLER_FUNCTIONS(UINT64)

‎src/google/protobuf/map_unittest.proto‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,3 +101,7 @@ message MessageContainingMapCalledEntry {
101101
message TestRecursiveMapMessage {
102102
map<string, TestRecursiveMapMessage> a = 1;
103103
}
104+
105+
message TestI32StrMap {
106+
map<int32, string> m_32_str = 1;
107+
}

‎src/google/protobuf/reflection_ops.cc‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,13 @@ bool ReflectionOps::IsInitialized(const Message& message) {
262262
std::vector<const FieldDescriptor*> fields;
263263
// Should be safe to skip stripped fields because required fields are not
264264
// stripped.
265-
reflection->ListFields(message, &fields);
265+
if (descriptor->options().map_entry()) {
266+
// MapEntry objects always check the value regardless of has bit.
267+
// We don't need to bother with the key.
268+
fields = {descriptor->map_value()};
269+
} else {
270+
reflection->ListFields(message, &fields);
271+
}
266272
for (const FieldDescriptor* field : fields) {
267273
if (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) {
268274

0 commit comments

Comments
 (0)