Skip to content

Commit

Permalink
Change upb C generated map iteration function to not hand out MapEntr…
Browse files Browse the repository at this point in the history 
…y pointers. (#20462)

Users relying on internal upb functions may be broken as these are meant to be *internal*. In the interim,  users may need to update their code by using the new Map iteration API.

PiperOrigin-RevId: 728363555

Co-authored-by: Protobuf Team Bot <protobuf-github-bot@google.com>
  • Loading branch information
@zhangskz @protobuf-github-bot
zhangskz and protobuf-github-bot authored Feb 25, 2025
1 parent 8892adc commit 503abcc
Show file tree
Hide file tree
Showing 3 changed files with 87 additions and 116 deletions.
88 changes: 41 additions & 47 deletions upb/message/copy_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,55 +191,49 @@ TEST(GeneratedCode, DeepCloneMessageMapField) {
protobuf_test_messages_proto2_TestAllTypesProto2_NestedMessage_set_a(nested,
0);
upb_Arena_Free(source_arena);
size_t iter = kUpb_Map_Begin;
// Test map<int32, int32>.
const protobuf_test_messages_proto2_TestAllTypesProto2_MapInt32DoubleEntry*
int32_double_entry =
protobuf_test_messages_proto2_TestAllTypesProto2_map_int32_double_next(
clone, &iter);
ASSERT_NE(int32_double_entry, nullptr);
EXPECT_EQ(
protobuf_test_messages_proto2_TestAllTypesProto2_MapInt32DoubleEntry_key(
int32_double_entry),
12);
EXPECT_EQ(
protobuf_test_messages_proto2_TestAllTypesProto2_MapInt32DoubleEntry_value(
int32_double_entry),
1200.5);
// Test map<int32, double>.
{
int32_t key;
double value;
size_t iter = kUpb_Map_Begin;

ASSERT_TRUE(
protobuf_test_messages_proto2_TestAllTypesProto2_map_int32_double_next(
clone, &key, &value, &iter));
EXPECT_EQ(key, 12);
EXPECT_EQ(value, 1200.5);
}

// Test map<string, string>.
iter = kUpb_Map_Begin;
const protobuf_test_messages_proto2_TestAllTypesProto2_MapStringStringEntry*
string_string_entry =
protobuf_test_messages_proto2_TestAllTypesProto2_map_string_string_next(
clone, &iter);
ASSERT_NE(string_string_entry, nullptr);
EXPECT_TRUE(upb_StringView_IsEqual(
protobuf_test_messages_proto2_TestAllTypesProto2_MapStringStringEntry_key(
string_string_entry),
upb_StringView_FromString("key1")));
EXPECT_TRUE(upb_StringView_IsEqual(
protobuf_test_messages_proto2_TestAllTypesProto2_MapStringStringEntry_value(
string_string_entry),
upb_StringView_FromString("value1")));
{
upb_StringView key;
upb_StringView value;
size_t iter = kUpb_Map_Begin;

ASSERT_TRUE(
protobuf_test_messages_proto2_TestAllTypesProto2_map_string_string_next(
clone, &key, &value, &iter));
EXPECT_TRUE(upb_StringView_IsEqual(key, upb_StringView_FromString("key1")));
EXPECT_TRUE(
upb_StringView_IsEqual(value, upb_StringView_FromString("value1")));
}

// Test map<string, NestedMessage>.
iter = kUpb_Map_Begin;
const protobuf_test_messages_proto2_TestAllTypesProto2_MapStringNestedMessageEntry*
nested_message_entry =
protobuf_test_messages_proto2_TestAllTypesProto2_map_string_nested_message_next(
clone, &iter);
ASSERT_NE(nested_message_entry, nullptr);
EXPECT_TRUE(upb_StringView_IsEqual(
protobuf_test_messages_proto2_TestAllTypesProto2_MapStringNestedMessageEntry_key(
nested_message_entry),
upb_StringView_FromString("nestedkey1")));
const protobuf_test_messages_proto2_TestAllTypesProto2_NestedMessage*
cloned_nested =
protobuf_test_messages_proto2_TestAllTypesProto2_MapStringNestedMessageEntry_value(
nested_message_entry);
ASSERT_NE(cloned_nested, nullptr);
EXPECT_EQ(protobuf_test_messages_proto2_TestAllTypesProto2_NestedMessage_a(
cloned_nested),
kTestNestedInt32);
{
upb_StringView key;
const protobuf_test_messages_proto2_TestAllTypesProto2_NestedMessage* value;
size_t iter = kUpb_Map_Begin;
ASSERT_TRUE(
protobuf_test_messages_proto2_TestAllTypesProto2_map_string_nested_message_next(
clone, &key, &value, &iter));
EXPECT_TRUE(
upb_StringView_IsEqual(key, upb_StringView_FromString("nestedkey1")));
ASSERT_NE(value, nullptr);
EXPECT_EQ(
protobuf_test_messages_proto2_TestAllTypesProto2_NestedMessage_a(value),
kTestNestedInt32);
}

upb_Arena_Free(arena);
}

Expand Down
95 changes: 31 additions & 64 deletions upb/test/test_generated_code.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -505,15 +505,16 @@ static void check_string_map_empty(
0,
protobuf_test_messages_proto3_TestAllTypesProto3_map_string_string_size(
msg));

upb_StringView key;
upb_StringView val;
EXPECT_FALSE(
protobuf_test_messages_proto3_TestAllTypesProto3_map_string_string_next(
msg, &iter));
msg, &key, &val, &iter));
}

static void check_string_map_one_entry(
protobuf_test_messages_proto3_TestAllTypesProto3* msg) {
const protobuf_test_messages_proto3_TestAllTypesProto3_MapStringStringEntry*
const_ent;
size_t iter;
upb_StringView str;

Expand All @@ -532,23 +533,15 @@ static void check_string_map_one_entry(

/* Test that iteration reveals a single k/v pair in the map. */
iter = kUpb_Map_Begin;
const_ent =
protobuf_test_messages_proto3_TestAllTypesProto3_map_string_string_next(
msg, &iter);
ASSERT_NE(nullptr, const_ent);
EXPECT_TRUE(upb_StringView_IsEqual(
test_str_view,
protobuf_test_messages_proto3_TestAllTypesProto3_MapStringStringEntry_key(
const_ent)));
EXPECT_TRUE(upb_StringView_IsEqual(
test_str_view2,
protobuf_test_messages_proto3_TestAllTypesProto3_MapStringStringEntry_value(
const_ent)));

const_ent =
upb_StringView key;
upb_StringView val;
protobuf_test_messages_proto3_TestAllTypesProto3_map_string_string_next(
msg, &key, &val, &iter);
EXPECT_TRUE(upb_StringView_IsEqual(test_str_view, key));
EXPECT_TRUE(upb_StringView_IsEqual(test_str_view2, val));
EXPECT_FALSE(
protobuf_test_messages_proto3_TestAllTypesProto3_map_string_string_next(
msg, &iter);
EXPECT_EQ(nullptr, const_ent);
msg, &key, &val, &iter));
}

TEST(GeneratedCode, StringDoubleMap) {
Expand Down Expand Up @@ -580,8 +573,6 @@ TEST(GeneratedCode, StringMap) {
upb_Arena* arena = upb_Arena_New();
protobuf_test_messages_proto3_TestAllTypesProto3* msg =
protobuf_test_messages_proto3_TestAllTypesProto3_new(arena);
const protobuf_test_messages_proto3_TestAllTypesProto3_MapStringStringEntry*
const_ent;
size_t iter, count;

check_string_map_empty(msg);
Expand Down Expand Up @@ -615,18 +606,11 @@ TEST(GeneratedCode, StringMap) {
/* Test iteration */
iter = kUpb_Map_Begin;
count = 0;

upb_StringView key;
upb_StringView val;
while (
(const_ent =
protobuf_test_messages_proto3_TestAllTypesProto3_map_string_string_next(
msg, &iter)) != nullptr) {
upb_StringView key =
protobuf_test_messages_proto3_TestAllTypesProto3_MapStringStringEntry_key(
const_ent);
upb_StringView val =
protobuf_test_messages_proto3_TestAllTypesProto3_MapStringStringEntry_value(
const_ent);

protobuf_test_messages_proto3_TestAllTypesProto3_map_string_string_next(
msg, &key, &val, &iter)) {
count++;
if (upb_StringView_IsEqual(key, test_str_view)) {
EXPECT_TRUE(upb_StringView_IsEqual(val, test_str_view2));
Expand All @@ -652,16 +636,18 @@ static void check_int32_map_empty(
EXPECT_EQ(
0, protobuf_test_messages_proto3_TestAllTypesProto3_map_int32_int32_size(
msg));

int32_t key;
int32_t val;
EXPECT_FALSE(
protobuf_test_messages_proto3_TestAllTypesProto3_map_int32_int32_next(
msg, &iter));
msg, &key, &val, &iter));
}

static void check_int32_map_one_entry(
protobuf_test_messages_proto3_TestAllTypesProto3* msg) {
const protobuf_test_messages_proto3_TestAllTypesProto3_MapInt32Int32Entry*
const_ent;
size_t iter;
int32_t key;
int32_t val;

EXPECT_EQ(
Expand All @@ -678,31 +664,20 @@ static void check_int32_map_one_entry(

/* Test that iteration reveals a single k/v pair in the map. */
iter = kUpb_Map_Begin;
const_ent =
EXPECT_TRUE(
protobuf_test_messages_proto3_TestAllTypesProto3_map_int32_int32_next(
msg, &iter);
ASSERT_NE(nullptr, const_ent);
EXPECT_EQ(
test_int32,
protobuf_test_messages_proto3_TestAllTypesProto3_MapInt32Int32Entry_key(
const_ent));
EXPECT_EQ(
test_int32_2,
protobuf_test_messages_proto3_TestAllTypesProto3_MapInt32Int32Entry_value(
const_ent));

const_ent =
msg, &key, &val, &iter));
EXPECT_EQ(test_int32, key);
EXPECT_EQ(test_int32_2, val);
EXPECT_FALSE(
protobuf_test_messages_proto3_TestAllTypesProto3_map_int32_int32_next(
msg, &iter);
EXPECT_EQ(nullptr, const_ent);
msg, &key, &val, &iter));
}

TEST(GeneratedCode, Int32Map) {
upb_Arena* arena = upb_Arena_New();
protobuf_test_messages_proto3_TestAllTypesProto3* msg =
protobuf_test_messages_proto3_TestAllTypesProto3_new(arena);
const protobuf_test_messages_proto3_TestAllTypesProto3_MapInt32Int32Entry*
const_ent;
size_t iter, count;

check_int32_map_empty(msg);
Expand Down Expand Up @@ -751,18 +726,10 @@ TEST(GeneratedCode, Int32Map) {
/* Test iteration */
iter = kUpb_Map_Begin;
count = 0;

while (
(const_ent =
protobuf_test_messages_proto3_TestAllTypesProto3_map_int32_int32_next(
msg, &iter)) != nullptr) {
int32_t key =
protobuf_test_messages_proto3_TestAllTypesProto3_MapInt32Int32Entry_key(
const_ent);
int32_t val =
protobuf_test_messages_proto3_TestAllTypesProto3_MapInt32Int32Entry_value(
const_ent);

int32_t key;
int32_t val;
while (protobuf_test_messages_proto3_TestAllTypesProto3_map_int32_int32_next(
msg, &key, &val, &iter)) {
count++;
if (key == test_int32) {
EXPECT_EQ(val, test_int32_2);
Expand Down
20 changes: 15 additions & 5 deletions upb_generator/c/generator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,10 @@ std::string MapValueCType(upb::FieldDefPtr map_field) {
return CType(map_field.message_type().map_value());
}

std::string MapValueCTypeConst(upb::FieldDefPtr map_field) {
return CTypeConst(map_field.message_type().map_value());
}

std::string MapKeyValueSize(upb_CType ctype, absl::string_view expr) {
return ctype == kUpb_CType_String || ctype == kUpb_CType_Bytes
? "0"
Expand Down Expand Up @@ -481,14 +485,20 @@ void GenerateMapGetters(upb::FieldDefPtr field, const DefPoolPair& pools,
MapValueSize(field, "*val"));
output(
R"cc(
UPB_INLINE $0 $1_$2_next(const $1* msg, size_t* iter) {
const upb_MiniTableField field = $3;
UPB_INLINE bool $0_$1_next(const $0* msg, $2* key, $3* val,
size_t* iter) {
const upb_MiniTableField field = $4;
const upb_Map* map = upb_Message_GetMap(UPB_UPCAST(msg), &field);
if (!map) return NULL;
return ($0)_upb_map_next(map, iter);
if (!map) return false;
upb_MessageValue k;
upb_MessageValue v;
if (!upb_Map_Next(map, &k, &v, iter)) return false;
memcpy(key, &k, sizeof(*key));
memcpy(val, &v, sizeof(*val));
return true;
}
)cc",
CTypeConst(field), msg_name, resolved_name,
msg_name, resolved_name, MapKeyCType(field), MapValueCTypeConst(field),
FieldInitializerStrong(pools, field, options));
// Generate private getter returning a upb_Map or NULL for immutable and
// a upb_Map for mutable.
Expand Down

0 comments on commit 503abcc

Please sign in to comment.