From 88f0a82a1888181d08c9bd105621d7c12dc058b3 Mon Sep 17 00:00:00 2001 From: Eric Salo Date: Wed, 15 Nov 2023 09:36:49 -0800 Subject: [PATCH] upb: normalize the accessor functions for hasbits and oneof_cases PiperOrigin-RevId: 582708011 --- upb/message/accessors.h | 2 +- upb/message/internal/accessors.h | 86 +++++++++++++-------- upb/message/promote.c | 4 +- upb/mini_descriptor/BUILD | 1 + upb/mini_descriptor/internal/encode_test.cc | 11 ++- upb/wire/decode.c | 4 +- upb/wire/encode.c | 10 +-- 7 files changed, 72 insertions(+), 46 deletions(-) diff --git a/upb/message/accessors.h b/upb/message/accessors.h index f8a9f9987b..f7cc1b7ac1 100644 --- a/upb/message/accessors.h +++ b/upb/message/accessors.h @@ -55,7 +55,7 @@ UPB_API_INLINE bool upb_Message_HasField(const upb_Message* msg, UPB_API_INLINE uint32_t upb_Message_WhichOneofFieldNumber( const upb_Message* message, const upb_MiniTableField* oneof_field) { UPB_ASSUME(_upb_MiniTableField_InOneOf(oneof_field)); - return _upb_getoneofcase_field(message, oneof_field); + return _upb_Message_GetOneofCase(message, oneof_field); } UPB_API_INLINE bool upb_Message_GetBool(const upb_Message* msg, diff --git a/upb/message/internal/accessors.h b/upb/message/internal/accessors.h index bc7dd59070..d80db81169 100644 --- a/upb/message/internal/accessors.h +++ b/upb/message/internal/accessors.h @@ -56,55 +56,75 @@ extern "C" { // Hasbit access /////////////////////////////////////////////////////////////// -UPB_INLINE size_t _upb_hasbit_ofs(size_t idx) { return idx / 8; } +UPB_INLINE size_t _upb_Hasbit_Offset(size_t index) { return index / 8; } -UPB_INLINE char _upb_hasbit_mask(size_t idx) { return 1 << (idx % 8); } +UPB_INLINE char _upb_Hasbit_Mask(size_t index) { return 1 << (index % 8); } -UPB_INLINE bool _upb_hasbit(const upb_Message* msg, size_t idx) { - return (*UPB_PTR_AT(msg, _upb_hasbit_ofs(idx), const char) & - _upb_hasbit_mask(idx)) != 0; +UPB_INLINE size_t _upb_Hasbit_Index(const upb_MiniTableField* f) { + UPB_ASSERT(f->presence > 0); + return f->presence; } -UPB_INLINE void _upb_sethas(const upb_Message* msg, size_t idx) { - (*UPB_PTR_AT(msg, _upb_hasbit_ofs(idx), char)) |= _upb_hasbit_mask(idx); +UPB_INLINE bool _upb_Message_GetHasbitByIndex(const upb_Message* msg, + size_t index) { + const size_t offset = _upb_Hasbit_Offset(index); + const char mask = _upb_Hasbit_Mask(index); + return (*UPB_PTR_AT(msg, offset, const char) & mask) != 0; } -UPB_INLINE void _upb_clearhas(const upb_Message* msg, size_t idx) { - (*UPB_PTR_AT(msg, _upb_hasbit_ofs(idx), char)) &= ~_upb_hasbit_mask(idx); +UPB_INLINE void _upb_Message_SetHasbitByIndex(const upb_Message* msg, + size_t index) { + const size_t offset = _upb_Hasbit_Offset(index); + const char mask = _upb_Hasbit_Mask(index); + (*UPB_PTR_AT(msg, offset, char)) |= mask; } -UPB_INLINE size_t _upb_Message_Hasidx(const upb_MiniTableField* f) { - UPB_ASSERT(f->presence > 0); - return f->presence; +UPB_INLINE void _upb_Message_ClearHasbitByIndex(const upb_Message* msg, + size_t index) { + const size_t offset = _upb_Hasbit_Offset(index); + const char mask = _upb_Hasbit_Mask(index); + (*UPB_PTR_AT(msg, offset, char)) &= ~mask; +} + +UPB_INLINE bool _upb_Message_GetHasbitByField(const upb_Message* msg, + const upb_MiniTableField* f) { + return _upb_Message_GetHasbitByIndex(msg, _upb_Hasbit_Index(f)); } -UPB_INLINE bool _upb_hasbit_field(const upb_Message* msg, - const upb_MiniTableField* f) { - return _upb_hasbit(msg, _upb_Message_Hasidx(f)); +UPB_INLINE void _upb_Message_SetHasbitByField(const upb_Message* msg, + const upb_MiniTableField* f) { + _upb_Message_SetHasbitByIndex(msg, _upb_Hasbit_Index(f)); } -UPB_INLINE void _upb_sethas_field(const upb_Message* msg, - const upb_MiniTableField* f) { - _upb_sethas(msg, _upb_Message_Hasidx(f)); +UPB_INLINE void _upb_Message_ClearHasbitByField(const upb_Message* msg, + const upb_MiniTableField* f) { + _upb_Message_ClearHasbitByIndex(msg, _upb_Hasbit_Index(f)); } // Oneof case access /////////////////////////////////////////////////////////// -UPB_INLINE size_t _upb_oneofcase_ofs(const upb_MiniTableField* f) { +UPB_INLINE size_t _upb_OneofCase_Offset(const upb_MiniTableField* f) { UPB_ASSERT(f->presence < 0); return ~(ptrdiff_t)f->presence; } -UPB_INLINE uint32_t* _upb_oneofcase_field(upb_Message* msg, - const upb_MiniTableField* f) { - return UPB_PTR_AT(msg, _upb_oneofcase_ofs(f), uint32_t); +UPB_INLINE uint32_t* _upb_Message_OneofCasePtr(upb_Message* msg, + const upb_MiniTableField* f) { + return UPB_PTR_AT(msg, _upb_OneofCase_Offset(f), uint32_t); } -UPB_INLINE uint32_t _upb_getoneofcase_field(const upb_Message* msg, - const upb_MiniTableField* f) { - return *_upb_oneofcase_field((upb_Message*)msg, f); +UPB_INLINE uint32_t _upb_Message_GetOneofCase(const upb_Message* msg, + const upb_MiniTableField* f) { + return *_upb_Message_OneofCasePtr((upb_Message*)msg, f); } +UPB_INLINE void _upb_Message_SetOneofCase(upb_Message* msg, + const upb_MiniTableField* f) { + *_upb_Message_OneofCasePtr(msg, f) = f->number; +} + +// TODO: implement _upb_Message_ClearOneofCase() + // LINT.ThenChange(GoogleInternalName2) UPB_INLINE bool _upb_MiniTableField_InOneOf(const upb_MiniTableField* field) { @@ -124,9 +144,9 @@ UPB_INLINE const void* _upb_MiniTableField_GetConstPtr( UPB_INLINE void _upb_Message_SetPresence(upb_Message* msg, const upb_MiniTableField* field) { if (field->presence > 0) { - _upb_sethas_field(msg, field); + _upb_Message_SetHasbitByField(msg, field); } else if (_upb_MiniTableField_InOneOf(field)) { - *_upb_oneofcase_field(msg, field) = field->number; + _upb_Message_SetOneofCase(msg, field); } } @@ -217,9 +237,9 @@ UPB_INLINE bool _upb_Message_HasNonExtensionField( UPB_ASSERT(upb_MiniTableField_HasPresence(field)); UPB_ASSUME(!upb_MiniTableField_IsExtension(field)); if (_upb_MiniTableField_InOneOf(field)) { - return _upb_getoneofcase_field(msg, field) == field->number; + return _upb_Message_GetOneofCase(msg, field) == field->number; } else { - return _upb_hasbit_field(msg, field); + return _upb_Message_GetHasbitByField(msg, field); } } @@ -308,11 +328,11 @@ UPB_INLINE void _upb_Message_ClearExtensionField( UPB_INLINE void _upb_Message_ClearNonExtensionField( upb_Message* msg, const upb_MiniTableField* field) { if (field->presence > 0) { - _upb_clearhas(msg, _upb_Message_Hasidx(field)); + _upb_Message_ClearHasbitByField(msg, field); } else if (_upb_MiniTableField_InOneOf(field)) { - uint32_t* oneof_case = _upb_oneofcase_field(msg, field); - if (*oneof_case != field->number) return; - *oneof_case = 0; + uint32_t* ptr = _upb_Message_OneofCasePtr(msg, field); + if (*ptr != field->number) return; + *ptr = 0; } const char zeros[16] = {0}; _upb_MiniTable_CopyFieldData(_upb_MiniTableField_GetPtr(msg, field), zeros, diff --git a/upb/message/promote.c b/upb/message/promote.c index b414eaa80b..57aa832ad9 100644 --- a/upb/message/promote.c +++ b/upb/message/promote.c @@ -239,7 +239,7 @@ upb_UnknownToMessageRet upb_MiniTable_PromoteUnknownToMessage( UPB_ASSERT(upb_MiniTable_GetSubMessageTable(mini_table, field) == sub_mini_table); bool is_oneof = _upb_MiniTableField_InOneOf(field); - if (!is_oneof || _upb_getoneofcase_field(msg, field) == field->number) { + if (!is_oneof || _upb_Message_GetOneofCase(msg, field) == field->number) { UPB_ASSERT(upb_Message_GetMessage(msg, field, NULL) == NULL); } upb_UnknownToMessageRet ret; @@ -271,7 +271,7 @@ upb_UnknownToMessageRet upb_MiniTable_PromoteUnknownToMessage( } while (unknown.status == kUpb_FindUnknown_Ok); if (message) { if (is_oneof) { - *_upb_oneofcase_field(msg, field) = field->number; + _upb_Message_SetOneofCase(msg, field); } upb_Message_SetMessage(msg, mini_table, field, message); ret.message = message; diff --git a/upb/mini_descriptor/BUILD b/upb/mini_descriptor/BUILD index d64279197f..6c82039559 100644 --- a/upb/mini_descriptor/BUILD +++ b/upb/mini_descriptor/BUILD @@ -64,6 +64,7 @@ cc_test( "//upb:mem", "//upb:message_accessors_internal", "//upb:mini_table", + "//upb:mini_table_internal", "//upb:wire", "@com_google_absl//absl/container:flat_hash_set", ], diff --git a/upb/mini_descriptor/internal/encode_test.cc b/upb/mini_descriptor/internal/encode_test.cc index dcb33b5aae..674ba46a86 100644 --- a/upb/mini_descriptor/internal/encode_test.cc +++ b/upb/mini_descriptor/internal/encode_test.cc @@ -7,13 +7,16 @@ #include "upb/mini_descriptor/internal/encode.hpp" +#include +#include + #include #include -#include #include #include "absl/container/flat_hash_set.h" #include "google/protobuf/descriptor.h" +#include "upb/base/descriptor_constants.h" #include "upb/base/status.hpp" #include "upb/mem/arena.hpp" #include "upb/message/internal/accessors.h" @@ -21,7 +24,9 @@ #include "upb/mini_descriptor/internal/base92.h" #include "upb/mini_descriptor/internal/modifiers.h" #include "upb/mini_table/enum.h" -#include "upb/wire/decode.h" +#include "upb/mini_table/field.h" +#include "upb/mini_table/internal/field.h" +#include "upb/mini_table/message.h" // begin:google_only // #include "testing/fuzzing/fuzztest.h" @@ -147,7 +152,7 @@ TEST_P(MiniTableTest, AllScalarTypesOneof) { // For a oneof all fields have the same offset. EXPECT_EQ(table->fields[0].offset, f->offset); // All presence fields should point to the same oneof case offset. - size_t case_ofs = _upb_oneofcase_ofs(f); + size_t case_ofs = _upb_OneofCase_Offset(f); EXPECT_EQ(table->fields[0].presence, f->presence); EXPECT_TRUE(f->offset < table->size); EXPECT_TRUE(case_ofs < table->size); diff --git a/upb/wire/decode.c b/upb/wire/decode.c index b51bcb71d4..93c42344f2 100644 --- a/upb/wire/decode.c +++ b/upb/wire/decode.c @@ -671,10 +671,10 @@ static const char* _upb_Decoder_DecodeToSubMessage( /* Set presence if necessary. */ if (field->presence > 0) { - _upb_sethas_field(msg, field); + _upb_Message_SetHasbitByField(msg, field); } else if (field->presence < 0) { /* Oneof case */ - uint32_t* oneof_case = _upb_oneofcase_field(msg, field); + uint32_t* oneof_case = _upb_Message_OneofCasePtr(msg, field); if (op == kUpb_DecodeOp_SubMessage && *oneof_case != field->number) { memset(mem, 0, sizeof(void*)); } diff --git a/upb/wire/encode.c b/upb/wire/encode.c index 774c6ff50a..212ec05410 100644 --- a/upb/wire/encode.c +++ b/upb/wire/encode.c @@ -458,7 +458,7 @@ static bool encode_shouldencode(upb_encstate* e, const upb_Message* msg, const upb_MiniTableSub* subs, const upb_MiniTableField* f) { if (f->presence == 0) { - /* Proto3 presence or map/array. */ + // Proto3 presence or map/array. const void* mem = UPB_PTR_AT(msg, f->offset, void); switch (_upb_MiniTableField_GetRep(f)) { case kUpb_FieldRep_1Byte: { @@ -484,11 +484,11 @@ static bool encode_shouldencode(upb_encstate* e, const upb_Message* msg, UPB_UNREACHABLE(); } } else if (f->presence > 0) { - /* Proto2 presence: hasbit. */ - return _upb_hasbit_field(msg, f); + // Proto2 presence: hasbit. + return _upb_Message_GetHasbitByField(msg, f); } else { - /* Field is in a oneof. */ - return _upb_getoneofcase_field(msg, f) == f->number; + // Field is in a oneof. + return _upb_Message_GetOneofCase(msg, f) == f->number; } }