Chiebot-Mirror
/
protobuf
mirror of https://github.com/protocolbuffers/protobuf.git
8
0
Fork 0
Code Issues Projects Releases Wiki Activity

Fix some of the reflection based algorithms to handle Map entries properly.

Browse Source 
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
pull/14934/head
Protobuf Team Bot 12 months ago committed by Copybara-Service
parent 53411896ab
commit 2b45018be4
7 changed files with 62 additions and 102 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 7
      src/google/protobuf/compiler/cpp/message.cc
  2. 25
      src/google/protobuf/map_entry.h
  3. 4
      src/google/protobuf/map_proto3_unittest.proto
  4. 43
      src/google/protobuf/map_test.cc
  5. 71
      src/google/protobuf/map_type_handler.h
  6. 4
      src/google/protobuf/map_unittest.proto
  7. 6
      src/google/protobuf/reflection_ops.cc

7
src/google/protobuf/compiler/cpp/message.cc
Unescape View File

@ -1160,19 +1160,18 @@ void MessageGenerator::GenerateMapEntryClassDefinition(io::Printer* p) {
Formatter format(p);
absl::flat_hash_map<absl::string_view, std::string> 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$>;

25
src/google/protobuf/map_entry.h
Unescape View File

@ -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<size_t>(KeyTypeHandler::ByteSize(key()));
size += kTagSize + static_cast<size_t>(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<Derived>(arena);
}

4
src/google/protobuf/map_proto3_unittest.proto
Unescape View File

@ -21,3 +21,7 @@ message TestProto3BytesMap {
map<int32, bytes> map_bytes = 1;
map<int32, string> map_string = 2;
}
message TestI32StrMap {
map<int32, string> m_32_str = 1;
}

43
src/google/protobuf/map_test.cc
Unescape View File

@ -10,6 +10,7 @@
#include <array>
#include <cstddef>
#include <cstdint>
#include <memory>
#include <string>
#include <type_traits>
#include <utility>
@ -170,6 +171,48 @@ TEST(MapTest, CopyConstructMessagesWithArena) {
EXPECT_EQ(map1["2"].GetArena(), &arena);
}
TEST(MapTest, AlwaysSerializesBothEntries) {
for (const Message* prototype :
{static_cast<const Message*>(
&protobuf_unittest::TestI32StrMap::default_instance()),
static_cast<const Message*>(
&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> 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

71
src/google/protobuf/map_type_handler.h
Unescape View File

@ -93,16 +93,10 @@ class MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type> {
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<WireFormatLite::TYPE_MESSAGE, Type> {
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 <typename Type>
inline const Type&
MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::GetExternalReference(
const Type* value) {
return *value;
}
template <typename Type>
void MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::DeleteNoArena(
const Type* ptr) {
@ -448,29 +430,9 @@ inline Type* MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::EnsureMutable(
return *value;
}
template <typename Type>
inline const Type&
MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::DefaultIfNotInitialized(
const Type* value) {
return value != nullptr ? *value : *Type::internal_default_instance();
}
template <typename Type>
inline bool MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::IsInitialized(
Type* value) {
return value ? value->IsInitialized() : false;
}
// Definition for string/bytes handler
#define STRING_OR_BYTES_HANDLER_FUNCTIONS(FieldType) \
template <typename Type> \
inline const typename MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
Type>::MapEntryAccessorType& \
MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
Type>::GetExternalReference(const TypeOnMemory& value) { \
return value.Get(); \
} \
template <typename Type> \
void MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::DeleteNoArena( \
TypeOnMemory& value) { \
@ -489,32 +451,12 @@ inline bool MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::IsInitialized(
MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::EnsureMutable( \
TypeOnMemory* value, Arena* arena) { \
return value->Mutable(arena); \
} \
template <typename Type> \
inline const typename MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
Type>::MapEntryAccessorType& \
MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
Type>::DefaultIfNotInitialized(const TypeOnMemory& value) { \
return value.Get(); \
} \
template <typename Type> \
inline bool \
MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::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 <typename Type> \
inline const typename MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
Type>::MapEntryAccessorType& \
MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
Type>::GetExternalReference(const TypeOnMemory& value) { \
return value; \
} \
template <typename Type> \
inline void MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
Type>::DeleteNoArena(TypeOnMemory& /* x */) {} \
@ -530,19 +472,6 @@ STRING_OR_BYTES_HANDLER_FUNCTIONS(BYTES)
MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::EnsureMutable( \
TypeOnMemory* value, Arena* /* arena */) { \
return value; \
} \
template <typename Type> \
inline const typename MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
Type>::MapEntryAccessorType& \
MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
Type>::DefaultIfNotInitialized(const TypeOnMemory& value) { \
return value; \
} \
template <typename Type> \
inline bool \
MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::IsInitialized( \
const TypeOnMemory& /* value */) { \
return true; \
}
PRIMITIVE_HANDLER_FUNCTIONS(INT64)
PRIMITIVE_HANDLER_FUNCTIONS(UINT64)

4
src/google/protobuf/map_unittest.proto
Unescape View File

@ -101,3 +101,7 @@ message MessageContainingMapCalledEntry {
message TestRecursiveMapMessage {
map<string, TestRecursiveMapMessage> a = 1;
}
message TestI32StrMap {
map<int32, string> m_32_str = 1;
}

6
src/google/protobuf/reflection_ops.cc
Unescape View File

@ -262,7 +262,13 @@ bool ReflectionOps::IsInitialized(const Message& message) {
std::vector<const FieldDescriptor*> fields;
// Should be safe to skip stripped fields because required fields are not
// stripped.
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) {

Write Preview
Loading…
Cancel
Save