From 0606564fd5a1b11bdcfe80f5052405908d8126c9 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Thu, 30 Dec 2021 19:08:01 -0800 Subject: [PATCH] Addressed PR comments. --- python/convert.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/python/convert.c b/python/convert.c index 6c67ff98bd..fe22126501 100644 --- a/python/convert.c +++ b/python/convert.c @@ -319,15 +319,31 @@ bool PyUpb_Array_IsEqual(const upb_array *arr1, const upb_array *arr2, bool PyUpb_Message_IsEqual(const upb_msg *msg1, const upb_msg *msg2, const upb_msgdef *m) { - const upb_symtab* symtab = upb_filedef_symtab(upb_msgdef_file(m)); - size_t iter1 = UPB_MSG_BEGIN; - size_t iter2 = UPB_MSG_BEGIN; - if (msg1 == msg2) return true; if (upb_msg_extcount(msg1) != upb_msg_extcount(msg2)) return false; + // Compare messages field-by-field. This is slightly tricky, because while + // we can iterate over normal fields in a predictable order, the extension + // order is unpredictable and may be different between msg1 and msg2. + // So we use the following strategy: + // 1. Iterate over all msg1 fields (including extensions). + // 2. For non-extension fields, we find the corresponding field by simply + // using upb_msg_next(msg2). If the two messages have the same set of + // fields, this will yield the same field. + // 3. For extension fields, we have to actually search for the corresponding + // field, which we do with upb_msg_get(msg2, ext_f1). + // 4. Once iteration over msg1 is complete, we call upb_msg_next(msg2) one + // final time to verify that we have visited all of msg2's regular fields + // (we pass NULL for ext_dict so that iteration will *not* return + // extensions). + // + // We don't need to visit all of msg2's extensions, because we verified up + // front that both messages have the same number of extensions. + const upb_symtab* symtab = upb_filedef_symtab(upb_msgdef_file(m)); const upb_fielddef *f1, *f2; upb_msgval val1, val2; + size_t iter1 = UPB_MSG_BEGIN; + size_t iter2 = UPB_MSG_BEGIN; while (upb_msg_next(msg1, m, symtab, &f1, &val1, &iter1)) { if (upb_fielddef_isextension(f1)) { val2 = upb_msg_get(msg2, f1);