diff --git a/upb/mini_table.c b/upb/mini_table.c index 93eefa860b..237e35725c 100644 --- a/upb/mini_table.c +++ b/upb/mini_table.c @@ -326,8 +326,9 @@ enum PresenceClass { kNoPresence = 0, kHasbitPresence = 1, kRequiredPresence = 2, - // Negative values refer to a specific oneof with that number. - // Positive values >=3 indicate that this field is in a oneof, and specify + kOneofBase = 3, + // Negative values refer to a specific oneof with that number. Positive + // values >= kOneofBase indicate that this field is in a oneof, and specify // the next field in this oneof's linked list. }; @@ -383,31 +384,34 @@ static bool upb_MiniTable_PushItem(upb_LayoutItemVector* vec, static bool upb_MiniTable_PushOneof(upb_LayoutItemVector* vec, upb_LayoutItem item) { - item.field_or_oneof = ~item.field_or_oneof; // Push oneof data. - item.is_case = false; + item.type = kUpb_LayoutItemType_OneofField; if (!upb_MiniTable_PushItem(vec, item)) return false; // Push oneof case. item.rep = kUpb_FieldRep_4Byte; // Field Number. - item.is_case = true; + item.type = kUpb_LayoutItemType_OneofCase; return upb_MiniTable_PushItem(vec, item); } static bool upb_MiniTable_DecodeOneofs(const char** ptr, const char* end, upb_MiniTable* ret, upb_LayoutItemVector* vec) { - upb_LayoutItem item = {.rep = 0, .field_or_oneof = -1}; + upb_LayoutItem item = {.rep = 0, + .field_index = kUpb_LayoutItem_IndexSentinel}; while (*ptr < end) { char ch = *(*ptr)++; if (ch == kUpb_EncodedValue_FieldSeparator) { // Field separator, no action needed. } else if (ch == kUpb_EncodedValue_OneofSeparator) { // End of oneof. + if (item.field_index == kUpb_LayoutItem_IndexSentinel) { + return false; // Empty oneof. + } + item.field_index -= kOneofBase; if (!upb_MiniTable_PushOneof(vec, item)) return false; - item.field_or_oneof = -1; // Move to next oneof. + item.field_index = kUpb_LayoutItem_IndexSentinel; // Move to next oneof. } else { - const char* before = *ptr; uint32_t field_num = upb_MiniTable_DecodeBase92Varint( ptr, end, ch, upb_ToBase92(0), upb_ToBase92(63)); upb_MiniTable_Field* f = @@ -415,12 +419,14 @@ static bool upb_MiniTable_DecodeOneofs(const char** ptr, const char* end, if (!f) return false; // Oneof storage must be large enough to accommodate the largest member. item.rep = UPB_MAX(item.rep, f->mode >> kUpb_FieldRep_Shift); - f->offset = item.field_or_oneof; - item.field_or_oneof = f - ret->fields; + // Prepend this field to the linked list. + f->offset = item.field_index; + item.field_index = (f - ret->fields) + kOneofBase; } } // Push final oneof. + item.field_index -= kOneofBase; return upb_MiniTable_PushOneof(vec, item); } @@ -436,21 +442,20 @@ int upb_MiniTable_CompareFields(const void* _a, const void* _b) { // // The main goal of this is to reduce space lost to padding. if (a->rep != b->rep) return UPB_COMPARE_INTEGERS(a->rep, b->rep); - if (a->is_case != b->is_case) { - return UPB_COMPARE_INTEGERS(a->is_case, b->is_case); - } - return UPB_COMPARE_INTEGERS(b->field_or_oneof, a->field_or_oneof); + if (a->type != b->type) return UPB_COMPARE_INTEGERS(a->type, b->type); + return UPB_COMPARE_INTEGERS(b->field_index, a->field_index); } #undef UPB_COMPARE_INTEGERS static bool upb_MiniTable_SortLayoutItems(upb_MiniTable* table, upb_LayoutItemVector* vec) { - // Add items for all fields that are not in a oneof. + // Add items for all non-oneof fields (oneofs were already added). int n = table->field_count; for (int i = 0; i < n; i++) { - upb_MiniTable_Field* f = (upb_MiniTable_Field*)&table->fields[i]; - upb_LayoutItem item = {.field_or_oneof = i, + upb_MiniTable_Field* f = (void*)&table->fields[i]; + if (f->offset >= kOneofBase) continue; + upb_LayoutItem item = {.field_index = i, .rep = f->mode >> kUpb_FieldRep_Shift}; if (!upb_MiniTable_PushItem(vec, item)) return false; } @@ -571,13 +576,41 @@ size_t upb_MiniTable_Place(upb_MiniTable* table, upb_FieldRep rep) { static bool upb_MiniTable_AssignOffsets(upb_MiniTable* ret, upb_LayoutItemVector* vec) { upb_LayoutItem* end = vec->data + vec->size; + + // Compute offsets. for (upb_LayoutItem* item = vec->data; item < end; item++) { item->offset = upb_MiniTable_Place(ret, item->rep); } + + // Assign oneof case offsets. We must do these first, since assigning + // actual offsets will overwrite the links of the linked list. + for (upb_LayoutItem* item = vec->data; item < end; item++) { + if (item->type != kUpb_LayoutItemType_OneofCase) continue; + upb_MiniTable_Field* f = (void*)&ret->fields[item->field_index]; + while (true) { + f->presence = ~item->offset; + if (f->offset == kUpb_LayoutItem_IndexSentinel) break; + f = (void*)&ret->fields[f->offset - kOneofBase]; + } + } + + // Assign offsets. for (upb_LayoutItem* item = vec->data; item < end; item++) { - if (item->field_or_oneof >= 0) { - upb_MiniTable_Field *f = (void *)&ret->fields[item->field_or_oneof]; - f->offset = + upb_MiniTable_Field* f = (void*)&ret->fields[item->field_index]; + switch (item->type) { + case kUpb_LayoutItemType_OneofField: + while (true) { + uint16_t next_offset = f->offset; + f->offset = item->offset; + if (next_offset == kUpb_LayoutItem_IndexSentinel) break; + f = (void*)&ret->fields[next_offset - kOneofBase]; + } + break; + case kUpb_LayoutItemType_Field: + f->offset = item->offset; + break; + default: + break; } } return true; diff --git a/upb/mini_table_test.cc b/upb/mini_table_test.cc index f8abb89448..938e44f71e 100644 --- a/upb/mini_table_test.cc +++ b/upb/mini_table_test.cc @@ -206,7 +206,12 @@ TEST(MiniTable, AllScalarTypesOneof) { EXPECT_EQ(kUpb_FieldMode_Scalar, f->mode & kUpb_FieldMode_Mask); // 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); + EXPECT_EQ(table->fields[0].presence, f->presence); EXPECT_TRUE(f->offset < table->size); + EXPECT_TRUE(case_ofs < table->size); + EXPECT_TRUE(case_ofs != f->offset); } EXPECT_EQ(0, table->required_count); }