From cb55c4d7815bc330bf3ad8aaaf0b13b4fb5399c0 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Sun, 13 Mar 2022 23:09:59 -0700 Subject: [PATCH] Addressed PR comments. --- upb/mini_table.c | 47 +++++++++++++++++++++++------------------- upb/msg_internal.h | 6 ++---- upbc/protoc-gen-upb.cc | 29 ++++++++++++++------------ 3 files changed, 44 insertions(+), 38 deletions(-) diff --git a/upb/mini_table.c b/upb/mini_table.c index fbba8df9a9..05f4b1ebf9 100644 --- a/upb/mini_table.c +++ b/upb/mini_table.c @@ -111,10 +111,14 @@ char upb_FromBase92(uint8_t ch) { } bool upb_IsTypePackable(upb_FieldType type) { - static const unsigned kPackableTypes = - -1U & ~(1 << kUpb_FieldType_String) & ~(1U << kUpb_FieldType_Bytes) & - ~(1U << kUpb_FieldType_Message) & ~(1U << kUpb_FieldType_Group); - return (1 << type) & kPackableTypes; + // clang-format off + static const unsigned kUnpackableTypes = + (1 << kUpb_FieldType_String) | + (1 << kUpb_FieldType_Bytes) | + (1 << kUpb_FieldType_Message) | + (1 << kUpb_FieldType_Group); + // clang-format on + return (1 << type) & ~kUnpackableTypes; } /** upb_MtDataEncoder *********************************************************/ @@ -178,7 +182,6 @@ char* upb_MtDataEncoder_StartMessage(upb_MtDataEncoder* e, char* ptr, return upb_MtDataEncoder_PutModifier(e, ptr, msg_mod); } -#include char* upb_MtDataEncoder_PutField(upb_MtDataEncoder* e, char* ptr, upb_FieldType type, uint32_t field_num, uint64_t field_mod) { @@ -229,10 +232,12 @@ char* upb_MtDataEncoder_PutField(upb_MtDataEncoder* e, char* ptr, // are bit flags). encoded_type += kUpb_EncodedType_RepeatedBase; - bool field_is_packed = field_mod & kUpb_FieldModifier_IsPacked; - bool default_is_packed = in->msg_mod & kUpb_MessageModifier_DefaultIsPacked; - if (field_is_packed != default_is_packed && upb_IsTypePackable(type)) { - encoded_modifiers |= kUpb_EncodedFieldModifier_FlipPacked; + if (upb_IsTypePackable(type)) { + bool field_is_packed = field_mod & kUpb_FieldModifier_IsPacked; + bool default_is_packed = in->msg_mod & kUpb_MessageModifier_DefaultIsPacked; + if (field_is_packed != default_is_packed) { + encoded_modifiers |= kUpb_EncodedFieldModifier_FlipPacked; + } } } ptr = upb_MtDataEncoder_Put(e, ptr, encoded_type); @@ -374,18 +379,19 @@ static const char* upb_MiniTable_DecodeBase92Varint(upb_MtDecoder* d, static bool upb_MiniTable_HasSub(upb_MiniTable_Field* field, uint64_t msg_modifiers) { - if (field->descriptortype == kUpb_FieldType_Message || - field->descriptortype == kUpb_FieldType_Group) { - return true; - } else if (field->descriptortype == kUpb_FieldType_Enum) { - return true; - } else if (field->descriptortype == kUpb_FieldType_String) { - if (!(msg_modifiers & kUpb_MessageModifier_ValidateUtf8)) { - field->descriptortype = kUpb_FieldType_Bytes; + switch (field->descriptortype) { + case kUpb_FieldType_Message: + case kUpb_FieldType_Group: + case kUpb_FieldType_Enum: + return true; + case kUpb_FieldType_String: + if (!(msg_modifiers & kUpb_MessageModifier_ValidateUtf8)) { + field->descriptortype = kUpb_FieldType_Bytes; + } + return false; + default: return false; - } } - return false; } static bool upb_MtDecoder_FieldIsPackable(upb_MiniTable_Field* field) { @@ -474,7 +480,6 @@ static void upb_MiniTable_SetField(upb_MtDecoder* d, uint8_t ch, msg_modifiers); } -#include static void upb_MtDecoder_ModifyField(upb_MtDecoder* d, uint32_t message_modifiers, uint32_t field_modifiers, @@ -996,7 +1001,7 @@ upb_MiniTable_Enum* upb_MiniTable_BuildEnum(const char* data, size_t len, if (ptr != end) { size_t bytes = end - ptr; - if (bytes / 4 * 4 != bytes) { + if (bytes % 4 != 0) { upb_MtDecoder_ErrorFormat(&decoder, "Bytes should be a multiple of 4"); UPB_UNREACHABLE(); } diff --git a/upb/msg_internal.h b/upb/msg_internal.h index 4b57f48c01..e0be1352f7 100644 --- a/upb/msg_internal.h +++ b/upb/msg_internal.h @@ -84,12 +84,10 @@ typedef enum { kUpb_FieldMode_Map = 0, kUpb_FieldMode_Array = 1, kUpb_FieldMode_Scalar = 2, - } upb_FieldMode; -#define kUpb_FieldMode_Mask \ - 3 /* Mask to isolate the mode from upb_FieldRep. \ - */ +// Mask to isolate the upb_FieldMode from field.mode. +#define kUpb_FieldMode_Mask 3 /* Extra flags on the mode field. */ typedef enum { diff --git a/upbc/protoc-gen-upb.cc b/upbc/protoc-gen-upb.cc index f18feec3c5..c7118eb11b 100644 --- a/upbc/protoc-gen-upb.cc +++ b/upbc/protoc-gen-upb.cc @@ -389,11 +389,10 @@ class FilePlatformLayout { const char* AllocStr(const std::string& str); private: - typedef absl::flat_hash_map - TableMap; - typedef absl::flat_hash_map - ExtensionMap; + using TableMap = + absl::flat_hash_map; + using ExtensionMap = absl::flat_hash_map; upb::Arena arena_; TableMap table_map_; ExtensionMap extension_map_; @@ -420,8 +419,10 @@ void FilePlatformLayout::ResolveIntraFileReferences() { for (const auto& pair : table_map_) { upb_MiniTable* mt = pair.second; // First we properly resolve for defs within the file. - for (const auto& f : FieldNumberOrder(pair.first)) { + for (const auto* f : FieldNumberOrder(pair.first)) { if (f->message_type() && f->message_type()->file() == f->file()) { + // const_cast is safe because the mini-table is owned exclusively + // by us, and was allocated from an arena (known-writable memory). upb_MiniTable_Field* mt_f = const_cast( upb_MiniTable_FindFieldByNumber(mt, f->number())); upb_MiniTable* sub_mt = GetMiniTable(f->message_type()); @@ -456,18 +457,20 @@ std::string FilePlatformLayout::GetSub(upb_MiniTable_Sub sub) { default: return std::string("{.submsg = NULL}"); } - return std::string("ERROR in WriteSub"); + return std::string("ERROR in GetSub"); } void FilePlatformLayout::SetSubTableStrings() { for (const auto& pair : table_map_) { upb_MiniTable* mt = pair.second; - for (const auto& f : FieldNumberOrder(pair.first)) { + for (const auto* f : FieldNumberOrder(pair.first)) { upb_MiniTable_Field* mt_f = const_cast( upb_MiniTable_FindFieldByNumber(mt, f->number())); assert(mt_f); upb_MiniTable_Sub sub = PackSubForField(f, mt_f); if (IsNull(sub)) continue; + // const_cast is safe because the mini-table is owned exclusively + // by us, and was allocated from an arena (known-writable memory). *const_cast(&mt->subs[mt_f->submsg_index]) = sub; } } @@ -502,9 +505,9 @@ void FilePlatformLayout::BuildMiniTables(const protobuf::FileDescriptor* fd) { } void FilePlatformLayout::BuildExtensions(const protobuf::FileDescriptor* fd) { - auto sorted = SortedExtensions(fd); + std::vector sorted = SortedExtensions(fd); upb::Status status; - for (const auto& f : sorted) { + for (const auto* f : sorted) { upb::MtDataEncoder e; e.StartMessage(0); e.PutField(static_cast(f->type()), f->number(), @@ -517,10 +520,10 @@ void FilePlatformLayout::BuildExtensions(const protobuf::FileDescriptor* fd) { fprintf(stderr, "Error building mini-table: %s\n", status.error_message()); } + ABSL_ASSERT(ok); ext.extendee = reinterpret_cast( AllocStr(MessageInit(f->containing_type()))); ext.sub = PackSubForField(f, &ext.field); - ABSL_ASSERT(ok); } } @@ -545,7 +548,7 @@ upb_MiniTable* FilePlatformLayout::MakeRegularMiniTable( const protobuf::Descriptor* m) { upb::MtDataEncoder e; e.StartMessage(GetMessageModifiers(m)); - for (const auto& f : FieldNumberOrder(m)) { + for (const auto* f : FieldNumberOrder(m)) { e.PutField(static_cast(f->type()), f->number(), GetFieldModifiers(f)); } @@ -557,7 +560,7 @@ upb_MiniTable* FilePlatformLayout::MakeRegularMiniTable( e.PutOneofField(f->number()); } } - const auto& str = e.data(); + const std::string& str = e.data(); upb::Status status; upb_MiniTable* ret = upb_MiniTable_Build(str.data(), str.size(), platform_, arena_.ptr(), status.ptr());