From e34e3bd32855d0132815e9110bfee71721090dc6 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Thu, 22 Dec 2022 17:08:29 -0800 Subject: [PATCH] Unified accessor API for map getters and setters. This is part of the ongoing effort to remove any hard-coding of layout offsets into the generated code (except via `upb_MiniTableField` values). PiperOrigin-RevId: 497266785 --- protos_generator/tests/test_generated.cc | 2 - upb/collections/map.c | 31 +++---- upb/collections/map_gencode_util.h | 44 ++++++++++ upb/collections/map_internal.h | 7 -- upb/message/accessors.h | 53 ++++-------- upb/mini_table/common.c | 35 ++++++++ upb/mini_table/common.h | 35 +------- upbc/protoc-gen-upb.cc | 103 ++++++++--------------- 8 files changed, 151 insertions(+), 159 deletions(-) diff --git a/protos_generator/tests/test_generated.cc b/protos_generator/tests/test_generated.cc index 416d9f466c..558476c60a 100644 --- a/protos_generator/tests/test_generated.cc +++ b/protos_generator/tests/test_generated.cc @@ -425,9 +425,7 @@ TEST(CppGeneratedCode, MessageMapStringKeyAndInt32Value) { test_model.clear_str_to_int_map(); EXPECT_EQ(0, test_model.str_to_int_map_size()); test_model.set_str_to_int_map("first", 10); - EXPECT_EQ(1, test_model.str_to_int_map_size()); test_model.set_str_to_int_map("second", 20); - EXPECT_EQ(2, test_model.str_to_int_map_size()); auto result = test_model.get_str_to_int_map("second"); EXPECT_EQ(true, result.ok()); EXPECT_EQ(20, result.value()); diff --git a/upb/collections/map.c b/upb/collections/map.c index 23bbc8cec1..e27e8b8980 100644 --- a/upb/collections/map.c +++ b/upb/collections/map.c @@ -35,24 +35,25 @@ // Must be last. #include "upb/port/def.inc" -// Strings/bytes are special-cased in maps. -char _upb_Map_CTypeSizeTable[12] = { - [kUpb_CType_Bool] = 1, - [kUpb_CType_Float] = 4, - [kUpb_CType_Int32] = 4, - [kUpb_CType_UInt32] = 4, - [kUpb_CType_Enum] = 4, - [kUpb_CType_Message] = sizeof(void*), - [kUpb_CType_Double] = 8, - [kUpb_CType_Int64] = 8, - [kUpb_CType_UInt64] = 8, - [kUpb_CType_String] = UPB_MAPTYPE_STRING, - [kUpb_CType_Bytes] = UPB_MAPTYPE_STRING, +/* Strings/bytes are special-cased in maps. */ +static char _upb_CTypeo_mapsize[12] = { + 0, + 1, /* kUpb_CType_Bool */ + 4, /* kUpb_CType_Float */ + 4, /* kUpb_CType_Int32 */ + 4, /* kUpb_CType_UInt32 */ + 4, /* kUpb_CType_Enum */ + sizeof(void*), /* kUpb_CType_Message */ + 8, /* kUpb_CType_Double */ + 8, /* kUpb_CType_Int64 */ + 8, /* kUpb_CType_UInt64 */ + 0, /* kUpb_CType_String */ + 0, /* kUpb_CType_Bytes */ }; upb_Map* upb_Map_New(upb_Arena* a, upb_CType key_type, upb_CType value_type) { - return _upb_Map_New(a, _upb_Map_CTypeSize(key_type), - _upb_Map_CTypeSize(value_type)); + return _upb_Map_New(a, _upb_CTypeo_mapsize[key_type], + _upb_CTypeo_mapsize[value_type]); } size_t upb_Map_Size(const upb_Map* map) { return _upb_Map_Size(map); } diff --git a/upb/collections/map_gencode_util.h b/upb/collections/map_gencode_util.h index 794d76b9f4..59764c96b2 100644 --- a/upb/collections/map_gencode_util.h +++ b/upb/collections/map_gencode_util.h @@ -41,6 +41,50 @@ extern "C" { // Message map operations, these get the map from the message first. +UPB_INLINE size_t _upb_msg_map_size(const upb_Message* msg, size_t ofs) { + upb_Map* map = *UPB_PTR_AT(msg, ofs, upb_Map*); + return map ? _upb_Map_Size(map) : 0; +} + +UPB_INLINE bool _upb_msg_map_get(const upb_Message* msg, size_t ofs, + const void* key, size_t key_size, void* val, + size_t val_size) { + upb_Map* map = *UPB_PTR_AT(msg, ofs, upb_Map*); + if (!map) return false; + return _upb_Map_Get(map, key, key_size, val, val_size); +} + +UPB_INLINE void* _upb_msg_map_next(const upb_Message* msg, size_t ofs, + size_t* iter) { + upb_Map* map = *UPB_PTR_AT(msg, ofs, upb_Map*); + if (!map) return NULL; + return _upb_map_next(map, iter); +} + +UPB_INLINE bool _upb_msg_map_set(upb_Message* msg, size_t ofs, const void* key, + size_t key_size, void* val, size_t val_size, + upb_Arena* arena) { + upb_Map** map = UPB_PTR_AT(msg, ofs, upb_Map*); + if (!*map) { + *map = _upb_Map_New(arena, key_size, val_size); + } + return _upb_Map_Insert(*map, key, key_size, val, val_size, arena) != + kUpb_MapInsertStatus_OutOfMemory; +} + +UPB_INLINE bool _upb_msg_map_delete(upb_Message* msg, size_t ofs, + const void* key, size_t key_size) { + upb_Map* map = *UPB_PTR_AT(msg, ofs, upb_Map*); + if (!map) return false; + return _upb_Map_Delete(map, key, key_size); +} + +UPB_INLINE void _upb_msg_map_clear(upb_Message* msg, size_t ofs) { + upb_Map* map = *UPB_PTR_AT(msg, ofs, upb_Map*); + if (!map) return; + _upb_Map_Clear(map); +} + UPB_INLINE void _upb_msg_map_key(const void* msg, void* key, size_t size) { const upb_tabent* ent = (const upb_tabent*)msg; uint32_t u32len; diff --git a/upb/collections/map_internal.h b/upb/collections/map_internal.h index 51a169eeba..f235cb3ecd 100644 --- a/upb/collections/map_internal.h +++ b/upb/collections/map_internal.h @@ -151,13 +151,6 @@ UPB_INLINE size_t _upb_Map_Size(const upb_Map* map) { return map->table.t.count; } -// Strings/bytes are special-cased in maps. -extern char _upb_Map_CTypeSizeTable[12]; - -UPB_INLINE size_t _upb_Map_CTypeSize(upb_CType ctype) { - return _upb_Map_CTypeSizeTable[ctype]; -} - // Creates a new map on the given arena with this key/value type. upb_Map* _upb_Map_New(upb_Arena* a, size_t key_size, size_t value_size); diff --git a/upb/message/accessors.h b/upb/message/accessors.h index cafd0f8c18..697b5ed777 100644 --- a/upb/message/accessors.h +++ b/upb/message/accessors.h @@ -251,20 +251,6 @@ UPB_INLINE void _upb_Message_ClearNonExtensionField( field); } -UPB_INLINE upb_Map* _upb_MiniTable_GetOrCreateMutableMap( - upb_Message* msg, const upb_MiniTableField* field, size_t key_size, - size_t val_size, upb_Arena* arena) { - UPB_ASSUME(upb_IsRepeatedOrMap(field)); - upb_Map* map = NULL; - upb_Map* default_map_value = NULL; - _upb_Message_GetNonExtensionField(msg, field, &default_map_value, &map); - if (!map) { - map = _upb_Map_New(arena, key_size, val_size); - _upb_Message_SetNonExtensionField(msg, field, &map); - } - return map; -} - // EVERYTHING ABOVE THIS LINE IS INTERNAL - DO NOT USE ///////////////////////// UPB_API_INLINE void upb_Message_ClearField(upb_Message* msg, @@ -554,30 +540,27 @@ UPB_API_INLINE bool upb_MiniTableField_IsClosedEnum( return field->descriptortype == kUpb_FieldType_Enum; } -UPB_API_INLINE const upb_Map* upb_Message_GetMap( - const upb_Message* msg, const upb_MiniTableField* field) { - UPB_ASSUME(upb_FieldMode_Get(field) == kUpb_FieldMode_Map); - const upb_Map* ret; - const upb_Map* default_val = NULL; - _upb_Message_GetNonExtensionField(msg, field, &default_val, &ret); - return ret; -} - -// TODO: rename to GetOrCreateMutableMap UPB_API_INLINE upb_Map* upb_MiniTable_GetMutableMap( upb_Message* msg, const upb_MiniTable* map_entry_mini_table, const upb_MiniTableField* field, upb_Arena* arena) { - UPB_ASSERT(field->descriptortype == kUpb_FieldType_Message || - field->descriptortype == kUpb_FieldType_Group); - const upb_MiniTableField* map_entry_key_field = - &map_entry_mini_table->fields[0]; - const upb_MiniTableField* map_entry_value_field = - &map_entry_mini_table->fields[1]; - return _upb_MiniTable_GetOrCreateMutableMap( - msg, field, - _upb_Map_CTypeSize(upb_MiniTableField_CType(map_entry_key_field)), - _upb_Map_CTypeSize(upb_MiniTableField_CType(map_entry_value_field)), - arena); + UPB_ASSERT(map_entry_mini_table != NULL); + UPB_ASSUME(upb_IsRepeatedOrMap(field)); + upb_Map* map = NULL; + upb_Map* default_map_value = NULL; + _upb_Message_GetNonExtensionField(msg, field, &default_map_value, &map); + if (!map) { + // Allocate map. + UPB_ASSERT(field->descriptortype == kUpb_FieldType_Message || + field->descriptortype == kUpb_FieldType_Group); + const upb_MiniTableField* map_entry_key_field = + &map_entry_mini_table->fields[0]; + const upb_MiniTableField* map_entry_value_field = + &map_entry_mini_table->fields[1]; + map = upb_Map_New(arena, upb_MiniTableField_CType(map_entry_key_field), + upb_MiniTableField_CType(map_entry_value_field)); + _upb_Message_SetNonExtensionField(msg, field, &map); + } + return map; } // Updates a map entry given an entry message. diff --git a/upb/mini_table/common.c b/upb/mini_table/common.c index 818bfa2a7c..2b84af1a7f 100644 --- a/upb/mini_table/common.c +++ b/upb/mini_table/common.c @@ -76,3 +76,38 @@ upb_FieldType upb_MiniTableField_Type(const upb_MiniTableField* field) { } return field->descriptortype; } + +upb_CType upb_MiniTableField_CType(const upb_MiniTableField* f) { + switch (f->descriptortype) { + case kUpb_FieldType_Double: + return kUpb_CType_Double; + case kUpb_FieldType_Float: + return kUpb_CType_Float; + case kUpb_FieldType_Int64: + case kUpb_FieldType_SInt64: + case kUpb_FieldType_SFixed64: + return kUpb_CType_Int64; + case kUpb_FieldType_Int32: + case kUpb_FieldType_SFixed32: + case kUpb_FieldType_SInt32: + return kUpb_CType_Int32; + case kUpb_FieldType_UInt64: + case kUpb_FieldType_Fixed64: + return kUpb_CType_UInt64; + case kUpb_FieldType_UInt32: + case kUpb_FieldType_Fixed32: + return kUpb_CType_UInt32; + case kUpb_FieldType_Enum: + return kUpb_CType_Enum; + case kUpb_FieldType_Bool: + return kUpb_CType_Bool; + case kUpb_FieldType_String: + return kUpb_CType_String; + case kUpb_FieldType_Bytes: + return kUpb_CType_Bytes; + case kUpb_FieldType_Group: + case kUpb_FieldType_Message: + return kUpb_CType_Message; + } + UPB_UNREACHABLE(); +} diff --git a/upb/mini_table/common.h b/upb/mini_table/common.h index bb632e3bed..0f94cbc2fd 100644 --- a/upb/mini_table/common.h +++ b/upb/mini_table/common.h @@ -58,40 +58,7 @@ UPB_API const upb_MiniTableField* upb_MiniTable_FindFieldByNumber( UPB_API upb_FieldType upb_MiniTableField_Type(const upb_MiniTableField* field); -UPB_API_INLINE upb_CType upb_MiniTableField_CType(const upb_MiniTableField* f) { - switch (f->descriptortype) { - case kUpb_FieldType_Double: - return kUpb_CType_Double; - case kUpb_FieldType_Float: - return kUpb_CType_Float; - case kUpb_FieldType_Int64: - case kUpb_FieldType_SInt64: - case kUpb_FieldType_SFixed64: - return kUpb_CType_Int64; - case kUpb_FieldType_Int32: - case kUpb_FieldType_SFixed32: - case kUpb_FieldType_SInt32: - return kUpb_CType_Int32; - case kUpb_FieldType_UInt64: - case kUpb_FieldType_Fixed64: - return kUpb_CType_UInt64; - case kUpb_FieldType_UInt32: - case kUpb_FieldType_Fixed32: - return kUpb_CType_UInt32; - case kUpb_FieldType_Enum: - return kUpb_CType_Enum; - case kUpb_FieldType_Bool: - return kUpb_CType_Bool; - case kUpb_FieldType_String: - return kUpb_CType_String; - case kUpb_FieldType_Bytes: - return kUpb_CType_Bytes; - case kUpb_FieldType_Group: - case kUpb_FieldType_Message: - return kUpb_CType_Message; - } - UPB_UNREACHABLE(); -} +UPB_API upb_CType upb_MiniTableField_CType(const upb_MiniTableField* field); UPB_API_INLINE bool upb_MiniTableField_IsExtension( const upb_MiniTableField* field) { diff --git a/upbc/protoc-gen-upb.cc b/upbc/protoc-gen-upb.cc index c0d9aa7ff7..6ce24beb8f 100644 --- a/upbc/protoc-gen-upb.cc +++ b/upbc/protoc-gen-upb.cc @@ -227,30 +227,6 @@ std::string CTypeConst(const protobuf::FieldDescriptor* field) { return CTypeInternal(field, true); } -std::string MapKeyCType(const protobuf::FieldDescriptor* map_field) { - return CType(map_field->message_type()->map_key()); -} - -std::string MapValueCType(const protobuf::FieldDescriptor* map_field) { - return CType(map_field->message_type()->map_value()); -} - -std::string MapKeySize(const protobuf::FieldDescriptor* map_field, - absl::string_view expr) { - return map_field->message_type()->map_key()->cpp_type() == - protobuf::FieldDescriptor::CPPTYPE_STRING - ? "0" - : absl::StrCat("sizeof(", expr, ")"); -} - -std::string MapValueSize(const protobuf::FieldDescriptor* map_field, - absl::string_view expr) { - return map_field->message_type()->map_value()->cpp_type() == - protobuf::FieldDescriptor::CPPTYPE_STRING - ? "0" - : absl::StrCat("sizeof(", expr, ")"); -} - std::string FieldInitializer(const FileLayout& layout, const protobuf::FieldDescriptor* field); @@ -460,39 +436,38 @@ void GenerateMapGetters(const protobuf::FieldDescriptor* field, const FileLayout& layout, absl::string_view msg_name, const NameToFieldDescriptorMap& field_names, Output& output) { + const protobuf::Descriptor* entry = field->message_type(); + const protobuf::FieldDescriptor* key = entry->FindFieldByNumber(1); + const protobuf::FieldDescriptor* val = entry->FindFieldByNumber(2); std::string resolved_name = ResolveFieldName(field, field_names); output( R"cc( UPB_INLINE size_t $0_$1_size(const $0* msg) { - const upb_MiniTableField field = $2; - const upb_Map* map = upb_Message_GetMap(msg, &field); - return map ? _upb_Map_Size(map) : 0; + return _upb_msg_map_size(msg, $2); } )cc", - msg_name, resolved_name, FieldInitializer(layout, field)); + msg_name, resolved_name, layout.GetFieldOffset(field)); output( R"cc( UPB_INLINE bool $0_$1_get(const $0* msg, $2 key, $3* val) { - const upb_MiniTableField field = $4; - const upb_Map* map = upb_Message_GetMap(msg, &field); - if (!map) return false; - return _upb_Map_Get(map, &key, $5, val, $6); + return _upb_msg_map_get(msg, $4, &key, $5, val, $6); } )cc", - msg_name, resolved_name, MapKeyCType(field), MapValueCType(field), - FieldInitializer(layout, field), MapKeySize(field, "key"), - MapValueSize(field, "*val")); + msg_name, resolved_name, CType(key), CType(val), + layout.GetFieldOffset(field), + key->cpp_type() == protobuf::FieldDescriptor::CPPTYPE_STRING + ? "0" + : "sizeof(key)", + val->cpp_type() == protobuf::FieldDescriptor::CPPTYPE_STRING + ? "0" + : "sizeof(*val)"); output( R"cc( UPB_INLINE $0 $1_$2_next(const $1* msg, size_t* iter) { - const upb_MiniTableField field = $3; - const upb_Map* map = upb_Message_GetMap(msg, &field); - if (!map) return NULL; - return ($0)_upb_map_next(map, iter); + return ($0)_upb_msg_map_next(msg, $3, iter); } )cc", - CTypeConst(field), msg_name, resolved_name, - FieldInitializer(layout, field)); + CTypeConst(field), msg_name, resolved_name, layout.GetFieldOffset(field)); } void GenerateMapEntryGetters(const protobuf::FieldDescriptor* field, @@ -564,50 +539,46 @@ void GenerateMapSetters(const protobuf::FieldDescriptor* field, const FileLayout& layout, absl::string_view msg_name, const NameToFieldDescriptorMap& field_names, Output& output) { + const protobuf::Descriptor* entry = field->message_type(); + const protobuf::FieldDescriptor* key = entry->FindFieldByNumber(1); + const protobuf::FieldDescriptor* val = entry->FindFieldByNumber(2); std::string resolved_name = ResolveFieldName(field, field_names); output( R"cc( - UPB_INLINE void $0_$1_clear($0* msg) { - const upb_MiniTableField field = $2; - upb_Map* map = (upb_Map*)upb_Message_GetMap(msg, &field); - if (!map) return; - _upb_Map_Clear(map); - } + UPB_INLINE void $0_$1_clear($0* msg) { _upb_msg_map_clear(msg, $2); } )cc", - msg_name, resolved_name, FieldInitializer(layout, field)); + msg_name, resolved_name, layout.GetFieldOffset(field)); output( R"cc( UPB_INLINE bool $0_$1_set($0* msg, $2 key, $3 val, upb_Arena* a) { - const upb_MiniTableField field = $4; - upb_Map* map = _upb_MiniTable_GetOrCreateMutableMap(msg, &field, $5, $6, a); - return _upb_Map_Insert(map, &key, $5, &val, $6, a) != - kUpb_MapInsertStatus_OutOfMemory; + return _upb_msg_map_set(msg, $4, &key, $5, &val, $6, a); } )cc", - msg_name, resolved_name, MapKeyCType(field), MapValueCType(field), - FieldInitializer(layout, field), MapKeySize(field, "key"), - MapValueSize(field, "val")); + msg_name, resolved_name, CType(key), CType(val), + layout.GetFieldOffset(field), + key->cpp_type() == protobuf::FieldDescriptor::CPPTYPE_STRING + ? "0" + : "sizeof(key)", + val->cpp_type() == protobuf::FieldDescriptor::CPPTYPE_STRING + ? "0" + : "sizeof(val)"); output( R"cc( UPB_INLINE bool $0_$1_delete($0* msg, $2 key) { - const upb_MiniTableField field = $3; - upb_Map* map = (upb_Map*)upb_Message_GetMap(msg, &field); - if (!map) return false; - return _upb_Map_Delete(map, &key, $4); + return _upb_msg_map_delete(msg, $3, &key, $4); } )cc", - msg_name, resolved_name, MapKeyCType(field), - FieldInitializer(layout, field), MapKeySize(field, "key")); + msg_name, resolved_name, CType(key), layout.GetFieldOffset(field), + key->cpp_type() == protobuf::FieldDescriptor::CPPTYPE_STRING + ? "0" + : "sizeof(key)"); output( R"cc( UPB_INLINE $0 $1_$2_nextmutable($1* msg, size_t* iter) { - const upb_MiniTableField field = $3; - upb_Map* map = (upb_Map*)upb_Message_GetMap(msg, &field); - if (!map) return NULL; - return ($0)_upb_map_next(map, iter); + return ($0)_upb_msg_map_next(msg, $3, iter); } )cc", - CType(field), msg_name, resolved_name, FieldInitializer(layout, field)); + CType(field), msg_name, resolved_name, layout.GetFieldOffset(field)); } void GenerateRepeatedSetters(const protobuf::FieldDescriptor* field,