From 45dfd77d8799000a55840b1af150db6629a3e972 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Thu, 22 Dec 2022 14:16:23 -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: 497238313 --- 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, 159 insertions(+), 151 deletions(-) diff --git a/protos_generator/tests/test_generated.cc b/protos_generator/tests/test_generated.cc index 558476c60a..416d9f466c 100644 --- a/protos_generator/tests/test_generated.cc +++ b/protos_generator/tests/test_generated.cc @@ -425,7 +425,9 @@ 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 e27e8b8980..23bbc8cec1 100644 --- a/upb/collections/map.c +++ b/upb/collections/map.c @@ -35,25 +35,24 @@ // Must be last. #include "upb/port/def.inc" -/* 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 */ +// 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, }; upb_Map* upb_Map_New(upb_Arena* a, upb_CType key_type, upb_CType value_type) { - return _upb_Map_New(a, _upb_CTypeo_mapsize[key_type], - _upb_CTypeo_mapsize[value_type]); + return _upb_Map_New(a, _upb_Map_CTypeSize(key_type), + _upb_Map_CTypeSize(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 59764c96b2..794d76b9f4 100644 --- a/upb/collections/map_gencode_util.h +++ b/upb/collections/map_gencode_util.h @@ -41,50 +41,6 @@ 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 f235cb3ecd..51a169eeba 100644 --- a/upb/collections/map_internal.h +++ b/upb/collections/map_internal.h @@ -151,6 +151,13 @@ 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 697b5ed777..cafd0f8c18 100644 --- a/upb/message/accessors.h +++ b/upb/message/accessors.h @@ -251,6 +251,20 @@ 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, @@ -540,27 +554,30 @@ 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(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; + 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); } // Updates a map entry given an entry message. diff --git a/upb/mini_table/common.c b/upb/mini_table/common.c index 2b84af1a7f..818bfa2a7c 100644 --- a/upb/mini_table/common.c +++ b/upb/mini_table/common.c @@ -76,38 +76,3 @@ 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 0f94cbc2fd..bb632e3bed 100644 --- a/upb/mini_table/common.h +++ b/upb/mini_table/common.h @@ -58,7 +58,40 @@ UPB_API const upb_MiniTableField* upb_MiniTable_FindFieldByNumber( UPB_API upb_FieldType upb_MiniTableField_Type(const upb_MiniTableField* field); -UPB_API upb_CType upb_MiniTableField_CType(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_INLINE bool upb_MiniTableField_IsExtension( const upb_MiniTableField* field) { diff --git a/upbc/protoc-gen-upb.cc b/upbc/protoc-gen-upb.cc index 6ce24beb8f..c0d9aa7ff7 100644 --- a/upbc/protoc-gen-upb.cc +++ b/upbc/protoc-gen-upb.cc @@ -227,6 +227,30 @@ 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); @@ -436,38 +460,39 @@ 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) { - return _upb_msg_map_size(msg, $2); + const upb_MiniTableField field = $2; + const upb_Map* map = upb_Message_GetMap(msg, &field); + return map ? _upb_Map_Size(map) : 0; } )cc", - msg_name, resolved_name, layout.GetFieldOffset(field)); + msg_name, resolved_name, FieldInitializer(layout, field)); output( R"cc( UPB_INLINE bool $0_$1_get(const $0* msg, $2 key, $3* val) { - return _upb_msg_map_get(msg, $4, &key, $5, val, $6); + 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); } )cc", - 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)"); + msg_name, resolved_name, MapKeyCType(field), MapValueCType(field), + FieldInitializer(layout, field), MapKeySize(field, "key"), + MapValueSize(field, "*val")); output( R"cc( UPB_INLINE $0 $1_$2_next(const $1* msg, size_t* iter) { - return ($0)_upb_msg_map_next(msg, $3, 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); } )cc", - CTypeConst(field), msg_name, resolved_name, layout.GetFieldOffset(field)); + CTypeConst(field), msg_name, resolved_name, + FieldInitializer(layout, field)); } void GenerateMapEntryGetters(const protobuf::FieldDescriptor* field, @@ -539,46 +564,50 @@ 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) { _upb_msg_map_clear(msg, $2); } + 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); + } )cc", - msg_name, resolved_name, layout.GetFieldOffset(field)); + msg_name, resolved_name, FieldInitializer(layout, field)); output( R"cc( UPB_INLINE bool $0_$1_set($0* msg, $2 key, $3 val, upb_Arena* a) { - return _upb_msg_map_set(msg, $4, &key, $5, &val, $6, 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; } )cc", - 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)"); + msg_name, resolved_name, MapKeyCType(field), MapValueCType(field), + FieldInitializer(layout, field), MapKeySize(field, "key"), + MapValueSize(field, "val")); output( R"cc( UPB_INLINE bool $0_$1_delete($0* msg, $2 key) { - return _upb_msg_map_delete(msg, $3, &key, $4); + 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); } )cc", - msg_name, resolved_name, CType(key), layout.GetFieldOffset(field), - key->cpp_type() == protobuf::FieldDescriptor::CPPTYPE_STRING - ? "0" - : "sizeof(key)"); + msg_name, resolved_name, MapKeyCType(field), + FieldInitializer(layout, field), MapKeySize(field, "key")); output( R"cc( UPB_INLINE $0 $1_$2_nextmutable($1* msg, size_t* iter) { - return ($0)_upb_msg_map_next(msg, $3, 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); } )cc", - CType(field), msg_name, resolved_name, layout.GetFieldOffset(field)); + CType(field), msg_name, resolved_name, FieldInitializer(layout, field)); } void GenerateRepeatedSetters(const protobuf::FieldDescriptor* field,