diff options
| -rw-r--r-- | tests/test_def.c | 68 | ||||
| -rw-r--r-- | upb/def.c | 183 | ||||
| -rw-r--r-- | upb/def.h | 69 |
3 files changed, 33 insertions, 287 deletions
diff --git a/tests/test_def.c b/tests/test_def.c index 29c459a..9859ca0 100644 --- a/tests/test_def.c +++ b/tests/test_def.c @@ -207,65 +207,28 @@ static upb_msgdef *upb_msgdef_newnamed(const char *name, void *owner) { return m; } -static upb_enumdef *upb_enumdef_newnamed(const char *name, void *owner) { - upb_enumdef *e = upb_enumdef_new(owner); - upb_enumdef_setfullname(e, name, NULL); - return e; -} - -static void test_replacement() { +static void test_replacement_fails() { + bool ok; upb_symtab *s = upb_symtab_new(&s); - upb_enumdef *e2; - upb_msgdef *m2; - upb_enumdef *e; upb_status status = UPB_STATUS_INIT; - upb_def *newdefs[3]; - upb_def *newdefs2[1]; - const upb_msgdef *m3; + upb_def *newdefs[2]; upb_msgdef *m = upb_msgdef_newnamed("MyMessage", &s); - upb_msgdef_addfield(m, newfield("field1", 1, UPB_TYPE_ENUM, - UPB_LABEL_OPTIONAL, ".MyEnum", &s), - &s, NULL); - m2 = upb_msgdef_newnamed("MyMessage2", &s); - e = upb_enumdef_newnamed("MyEnum", &s); - ASSERT_STATUS(upb_enumdef_addval(e, "VAL1", 1, &status), &status); + upb_msgdef *m2 = upb_msgdef_newnamed("MyMessage", &s); newdefs[0] = upb_msgdef_upcast_mutable(m); newdefs[1] = upb_msgdef_upcast_mutable(m2); - newdefs[2] = upb_enumdef_upcast_mutable(e); - ASSERT_STATUS(upb_symtab_add(s, newdefs, 3, &s, &status), &status); - - /* Try adding a new definition of MyEnum, MyMessage should get replaced with - * a new version. */ - e2 = upb_enumdef_newnamed("MyEnum", &s); - ASSERT_STATUS(upb_enumdef_addval(e2, "VAL1", 1, &status), &status); - newdefs2[0] = upb_enumdef_upcast_mutable(e2); - ASSERT_STATUS(upb_symtab_add(s, newdefs2, 1, &s, &status), &status); - - m3 = upb_symtab_lookupmsg(s, "MyMessage"); - ASSERT(m3); - /* Must be different because it points to MyEnum which was replaced. */ - ASSERT(m3 != m); + ok = upb_symtab_add(s, newdefs, 2, &s, &status); + ASSERT(ok == false); - m3 = upb_symtab_lookupmsg(s, "MyMessage2"); - /* Should be the same because it was not replaced, nor were any defs that - * are reachable from it. */ - ASSERT(m3 == m2); + /* Adding just one is ok. */ + ASSERT_STATUS(upb_symtab_add(s, newdefs, 1, &s, &status), &status); - upb_symtab_free(s); -} + /* Adding a conflicting one is not ok. */ + newdefs[0] = upb_msgdef_upcast_mutable(m2); + ok = upb_symtab_add(s, newdefs, 1, &s, &status); + ASSERT(ok == false); -static void test_cycles_in_replacement() { - upb_symtab *s = upb_symtab_new(&s); - upb_msgdef *m = upb_msgdef_newnamed("M", &s); - upb_status status = UPB_STATUS_INIT; - - upb_msgdef_addfield(m, newfield("m", 1, UPB_TYPE_MESSAGE, - UPB_LABEL_OPTIONAL, ".M", &s), - &s, NULL); - ASSERT_STATUS(upb_symtab_add(s, (upb_def**)&m, 1, &s, &status), &status); - ASSERT_STATUS(upb_symtab_add(s, NULL, 0, &s, &status), &status); upb_symtab_free(s); } @@ -365,7 +328,6 @@ static void test_partial_freeze() { static void test_descriptor_flags() { upb_msgdef *m = upb_msgdef_new(&m); - upb_msgdef *m2; upb_status s = UPB_STATUS_INIT; ASSERT(upb_msgdef_mapentry(m) == false); @@ -373,10 +335,7 @@ static void test_descriptor_flags() { ASSERT(upb_ok(&s)); upb_msgdef_setmapentry(m, true); ASSERT(upb_msgdef_mapentry(m) == true); - m2 = upb_msgdef_dup(m, &m2); - ASSERT(upb_msgdef_mapentry(m2) == true); upb_msgdef_unref(m, &m); - upb_msgdef_unref(m2, &m2); } static void test_mapentry_check() { @@ -482,8 +441,7 @@ int run_tests(int argc, char *argv[]) { test_symbol_resolution(); test_fielddef(); test_fielddef_unref(); - test_replacement(); - test_cycles_in_replacement(); + test_replacement_fails(); test_freeze_free(); test_partial_freeze(); test_noreftracking(); @@ -122,22 +122,6 @@ bool upb_def_setfullname(upb_def *def, const char *fullname, upb_status *s) { const upb_filedef *upb_def_file(const upb_def *d) { return d->file; } -upb_def *upb_def_dup(const upb_def *def, const void *o) { - switch (def->type) { - case UPB_DEF_MSG: - return upb_msgdef_upcast_mutable( - upb_msgdef_dup(upb_downcast_msgdef(def), o)); - case UPB_DEF_FIELD: - return upb_fielddef_upcast_mutable( - upb_fielddef_dup(upb_downcast_fielddef(def), o)); - case UPB_DEF_ENUM: - return upb_enumdef_upcast_mutable( - upb_enumdef_dup(upb_downcast_enumdef(def), o)); - default: - UPB_UNREACHABLE(); - } -} - static bool upb_def_init(upb_def *def, upb_deftype_t type, const struct upb_refcounted_vtbl *vtbl, const void *owner) { @@ -493,21 +477,6 @@ err2: return NULL; } -upb_enumdef *upb_enumdef_dup(const upb_enumdef *e, const void *owner) { - upb_enum_iter i; - upb_enumdef *new_e = upb_enumdef_new(owner); - if (!new_e) return NULL; - for(upb_enum_begin(&i, e); !upb_enum_done(&i); upb_enum_next(&i)) { - bool success = upb_enumdef_addval( - new_e, upb_enum_iter_name(&i),upb_enum_iter_number(&i), NULL); - if (!success) { - upb_enumdef_unref(new_e, owner); - return NULL; - } - } - return new_e; -} - bool upb_enumdef_freeze(upb_enumdef *e, upb_status *status) { upb_def *d = upb_enumdef_upcast_mutable(e); return upb_def_freeze(&d, 1, status); @@ -742,7 +711,8 @@ upb_fielddef *upb_fielddef_new(const void *o) { return f; } -upb_fielddef *upb_fielddef_dup(const upb_fielddef *f, const void *owner) { +static upb_fielddef *upb_fielddef_dup(const upb_fielddef *f, + const void *owner) { const char *srcname; upb_fielddef *newf = upb_fielddef_new(owner); if (!newf) return NULL; @@ -1457,7 +1427,9 @@ err2: return NULL; } -upb_msgdef *upb_msgdef_dup(const upb_msgdef *m, const void *owner) { +static upb_oneofdef *upb_oneofdef_dup(const upb_oneofdef *o, const void *owner); + +static upb_msgdef *upb_msgdef_dup(const upb_msgdef *m, const void *owner) { bool ok; upb_msg_field_iter i; upb_msg_oneof_iter o; @@ -1793,7 +1765,8 @@ err2: return NULL; } -upb_oneofdef *upb_oneofdef_dup(const upb_oneofdef *o, const void *owner) { +static upb_oneofdef *upb_oneofdef_dup(const upb_oneofdef *o, + const void *owner) { bool ok; upb_oneof_iter i; upb_oneofdef *newo = upb_oneofdef_new(owner); @@ -2209,115 +2182,7 @@ const upb_def *upb_symtab_resolve(const upb_symtab *s, const char *base, return ret; } -/* Starts a depth-first traversal at "def", recursing into any subdefs - * (ie. submessage types). Adds duplicates of existing defs to addtab - * wherever necessary, so that the resulting symtab will be consistent once - * addtab is added. - * - * More specifically, if any def D is found in the DFS that: - * - * 1. can reach a def that is being replaced by something in addtab, AND - * - * 2. is not itself being replaced already (ie. this name doesn't already - * exist in addtab) - * - * ...then a duplicate (new copy) of D will be added to addtab. - * - * Returns true if this happened for any def reachable from "def." - * - * It is slightly tricky to do this correctly in the presence of cycles. If we - * detect that our DFS has hit a cycle, we might not yet know if any SCCs on - * our stack can reach a def in addtab or not. Once we figure this out, that - * answer needs to apply to *all* defs in these SCCs, even if we visited them - * already. So a straight up one-pass cycle-detecting DFS won't work. - * - * To work around this problem, we traverse each SCC (which we already - * computed, since these defs are frozen) as a single node. We first compute - * whether the SCC as a whole can reach any def in addtab, then we dup (or not) - * the entire SCC. This requires breaking the encapsulation of upb_refcounted, - * since that is where we get the data about what SCC we are in. */ -static bool upb_resolve_dfs(const upb_def *def, upb_strtable *addtab, - const void *new_owner, upb_inttable *seen, - upb_status *s) { - upb_value v; - bool need_dup; - const upb_def *base; - const void* memoize_key; - - /* Memoize results of this function for efficiency (since we're traversing a - * DAG this is not needed to limit the depth of the search). - * - * We memoize by SCC instead of by individual def. */ - memoize_key = def->base.group; - - if (upb_inttable_lookupptr(seen, memoize_key, &v)) - return upb_value_getbool(v); - - /* Visit submessages for all messages in the SCC. */ - need_dup = false; - base = def; - do { - upb_value v; - const upb_msgdef *m; - - UPB_ASSERT(upb_def_isfrozen(def)); - if (def->type == UPB_DEF_FIELD) continue; - if (upb_strtable_lookup(addtab, upb_def_fullname(def), &v)) { - need_dup = true; - } - - /* For messages, continue the recursion by visiting all subdefs, but only - * ones in different SCCs. */ - m = upb_dyncast_msgdef(def); - if (m) { - upb_msg_field_iter i; - for(upb_msg_field_begin(&i, m); - !upb_msg_field_done(&i); - upb_msg_field_next(&i)) { - upb_fielddef *f = upb_msg_iter_field(&i); - const upb_def *subdef; - - if (!upb_fielddef_hassubdef(f)) continue; - subdef = upb_fielddef_subdef(f); - - /* Skip subdefs in this SCC. */ - if (def->base.group == subdef->base.group) continue; - - /* |= to avoid short-circuit; we need its side-effects. */ - need_dup |= upb_resolve_dfs(subdef, addtab, new_owner, seen, s); - if (!upb_ok(s)) return false; - } - } - } while ((def = (upb_def*)def->base.next) != base); - - if (need_dup) { - /* Dup all defs in this SCC that don't already have entries in addtab. */ - def = base; - do { - const char *name; - - if (def->type == UPB_DEF_FIELD) continue; - name = upb_def_fullname(def); - if (!upb_strtable_lookup(addtab, name, NULL)) { - upb_def *newdef = upb_def_dup(def, new_owner); - if (!newdef) goto oom; - newdef->came_from_user = false; - if (!upb_strtable_insert(addtab, name, upb_value_ptr(newdef))) - goto oom; - } - } while ((def = (upb_def*)def->base.next) != base); - } - - upb_inttable_insertptr(seen, memoize_key, upb_value_bool(need_dup)); - return need_dup; - -oom: - upb_status_seterrmsg(s, "out of memory"); - return false; -} - -/* TODO(haberman): we need a lot more testing of error conditions. - * The came_from_user stuff in particular is not tested. */ +/* TODO(haberman): we need a lot more testing of error conditions. */ static bool symtab_add(upb_symtab *s, upb_def *const*defs, size_t n, void *ref_donor, upb_refcounted *freeze_also, upb_status *status) { @@ -2329,7 +2194,6 @@ static bool symtab_add(upb_symtab *s, upb_def *const*defs, size_t n, upb_def **add_defs = NULL; size_t add_objs_size; upb_strtable addtab; - upb_inttable seen; if (n == 0 && !freeze_also) { return true; @@ -2372,12 +2236,15 @@ static bool symtab_add(upb_symtab *s, upb_def *const*defs, size_t n, upb_status_seterrf(status, "Conflicting defs named '%s'", fullname); goto err; } - /* We need this to back out properly, because if there is a failure we - * need to donate the ref back to the caller. */ - def->came_from_user = true; + if (upb_strtable_lookup(&s->symtab, fullname, NULL)) { + upb_status_seterrf(status, "Symtab already has a def named '%s'", + fullname); + goto err; + } upb_def_donateref(def, ref_donor, s); if (!upb_strtable_insert(&addtab, fullname, upb_value_ptr(def))) goto oom_err; + def->came_from_user = true; } } @@ -2429,17 +2296,6 @@ static bool symtab_add(upb_symtab *s, upb_def *const*defs, size_t n, } } - /* Add dups of any existing def that can reach a def with the same name as - * anything in our "add" set. */ - if (!upb_inttable_init(&seen, UPB_CTYPE_BOOL)) goto oom_err; - upb_strtable_begin(&iter, &s->symtab); - for (; !upb_strtable_done(&iter); upb_strtable_next(&iter)) { - upb_def *def = upb_value_getptr(upb_strtable_iter_value(&iter)); - upb_resolve_dfs(def, &addtab, s, &seen, status); - if (!upb_ok(status)) goto err; - } - upb_inttable_uninit(&seen); - /* Now using the table, resolve symbolic references for subdefs. */ upb_strtable_begin(&iter, &addtab); for (; !upb_strtable_done(&iter); upb_strtable_next(&iter)) { @@ -2533,18 +2389,11 @@ static bool symtab_add(upb_symtab *s, upb_def *const*defs, size_t n, oom_err: upb_status_seterrmsg(status, "out of memory"); err: { - /* For defs the user passed in, we need to donate the refs back. For defs - * we dup'd, we need to just unref them. */ + /* We need to donate the refs back. */ upb_strtable_begin(&iter, &addtab); for (; !upb_strtable_done(&iter); upb_strtable_next(&iter)) { upb_def *def = upb_value_getptr(upb_strtable_iter_value(&iter)); - bool came_from_user = def->came_from_user; - def->came_from_user = false; - if (came_from_user) { - upb_def_donateref(def, s, ref_donor); - } else { - upb_def_unref(def, s); - } + upb_def_donateref(def, s, ref_donor); } } upb_strtable_uninit(&addtab); @@ -79,8 +79,6 @@ class upb::Def { public: typedef upb_deftype_t Type; - Def* Dup(const void *owner) const; - /* upb::RefCounted methods like Ref()/Unref(). */ UPB_REFCOUNTED_CPPMETHODS @@ -126,9 +124,6 @@ class upb::Def { UPB_BEGIN_EXTERN_C -/* Native C API. */ -upb_def *upb_def_dup(const upb_def *def, const void *owner); - /* Include upb_refcounted methods like upb_def_ref()/upb_def_unref(). */ UPB_REFCOUNTED_CMETHODS(upb_def, upb_def_upcast) @@ -318,13 +313,6 @@ class upb::FieldDef { /* Returns NULL if memory allocation failed. */ static reffed_ptr<FieldDef> New(); - /* Duplicates the given field, returning NULL if memory allocation failed. - * When a fielddef is duplicated, the subdef (if any) is made symbolic if it - * wasn't already. If the subdef is set but has no name (which is possible - * since msgdefs are not required to have a name) the new fielddef's subdef - * will be unset. */ - FieldDef* Dup(const void* owner) const; - /* upb::RefCounted methods like Ref()/Unref(). */ UPB_REFCOUNTED_CPPMETHODS @@ -573,7 +561,6 @@ UPB_BEGIN_EXTERN_C /* Native C API. */ upb_fielddef *upb_fielddef_new(const void *owner); -upb_fielddef *upb_fielddef_dup(const upb_fielddef *f, const void *owner); /* Include upb_refcounted methods like upb_fielddef_ref(). */ UPB_REFCOUNTED_CMETHODS(upb_fielddef, upb_fielddef_upcast2) @@ -783,16 +770,6 @@ class upb::MessageDef { return FindOneofByName(str.c_str(), str.size()); } - /* Returns a new msgdef that is a copy of the given msgdef (and a copy of all - * the fields) but with any references to submessages broken and replaced - * with just the name of the submessage. Returns NULL if memory allocation - * failed. - * - * TODO(haberman): which is more useful, keeping fields resolved or - * unresolving them? If there's no obvious answer, Should this functionality - * just be moved into symtab.c? */ - MessageDef* Dup(const void* owner) const; - /* Is this message a map entry? */ void setmapentry(bool map_entry); bool mapentry() const; @@ -926,7 +903,6 @@ UPB_REFCOUNTED_CMETHODS(upb_msgdef, upb_msgdef_upcast2) bool upb_msgdef_freeze(upb_msgdef *m, upb_status *status); -upb_msgdef *upb_msgdef_dup(const upb_msgdef *m, const void *owner); const char *upb_msgdef_fullname(const upb_msgdef *m); const char *upb_msgdef_name(const upb_msgdef *m); int upb_msgdef_numoneofs(const upb_msgdef *m); @@ -1076,10 +1052,6 @@ class upb::EnumDef { * first one that was added. */ const char* FindValueByNumber(int32_t num) const; - /* Returns a new EnumDef with all the same values. The new EnumDef will be - * owned by the given owner. */ - EnumDef* Dup(const void* owner) const; - /* Iteration over name/value pairs. The order is undefined. * Adding an enum val invalidates any iterators. * @@ -1107,7 +1079,6 @@ UPB_BEGIN_EXTERN_C /* Native C API. */ upb_enumdef *upb_enumdef_new(const void *owner); -upb_enumdef *upb_enumdef_dup(const upb_enumdef *e, const void *owner); /* Include upb_refcounted methods like upb_enumdef_ref(). */ UPB_REFCOUNTED_CMETHODS(upb_enumdef, upb_enumdef_upcast2) @@ -1217,10 +1188,6 @@ class upb::OneofDef { /* Looks up by tag number. */ const FieldDef* FindFieldByNumber(uint32_t num) const; - /* Returns a new OneofDef with all the same fields. The OneofDef will be owned - * by the given owner. */ - OneofDef* Dup(const void* owner) const; - /* Iteration over fields. The order is undefined. */ class iterator : public std::iterator<std::forward_iterator_tag, FieldDef*> { public: @@ -1266,7 +1233,6 @@ UPB_BEGIN_EXTERN_C /* Native C API. */ upb_oneofdef *upb_oneofdef_new(const void *owner); -upb_oneofdef *upb_oneofdef_dup(const upb_oneofdef *o, const void *owner); /* Include upb_refcounted methods like upb_oneofdef_ref(). */ UPB_REFCOUNTED_CMETHODS(upb_oneofdef, upb_oneofdef_upcast) @@ -1472,21 +1438,12 @@ class upb::SymbolTable { * you ask for an iterator of MessageDef the iterated elements are strongly * typed as MessageDef*. */ - /* Adds the given mutable defs to the symtab, resolving all symbols - * (including enum default values) and finalizing the defs. Only one def per - * name may be in the list, but defs can replace existing defs in the symtab. + /* Adds the given mutable defs to the symtab, resolving all symbols (including + * enum default values) and finalizing the defs. Only one def per name may be + * in the list, and the defs may not duplicate any name already in the symtab. * All defs must have a name -- anonymous defs are not allowed. Anonymous * defs can still be frozen by calling upb_def_freeze() directly. * - * Any existing defs that can reach defs that are being replaced will - * themselves be replaced also, so that the resulting set of defs is fully - * consistent. - * - * This logic implemented in this method is a convenience; ultimately it - * calls some combination of upb_fielddef_setsubdef(), upb_def_dup(), and - * upb_freeze(), any of which the client could call themself. However, since - * the logic for doing so is nontrivial, we provide it here. - * * The entire operation either succeeds or fails. If the operation fails, * the symtab is unchanged, false is returned, and status indicates the * error. The caller passes a ref on all defs to the symtab (even if the @@ -1496,13 +1453,7 @@ class upb::SymbolTable { * leave the defs themselves partially resolved. Does this matter? If so we * could do a prepass that ensures that all symbols are resolvable and bail * if not, so we don't mutate anything until we know the operation will - * succeed. - * - * TODO(haberman): since the defs must be mutable, refining a frozen def - * requires making mutable copies of the entire tree. This is wasteful if - * only a few messages are changing. We may want to add a way of adding a - * tree of frozen defs to the symtab (perhaps an alternate constructor where - * you pass the root of the tree?) */ + * succeed. */ bool Add(Def*const* defs, size_t n, void* ref_donor, Status* status); bool Add(const std::vector<Def*>& defs, void *owner, Status* status) { @@ -1592,9 +1543,6 @@ UPB_INLINE const char* upb_safecstr(const std::string& str) { /* Inline C++ wrappers. */ namespace upb { -inline Def* Def::Dup(const void* owner) const { - return upb_def_dup(this, owner); -} inline Def::Type Def::def_type() const { return upb_def_type(this); } inline const char* Def::full_name() const { return upb_def_fullname(this); } inline const char* Def::name() const { return upb_def_name(this); } @@ -1644,9 +1592,6 @@ inline reffed_ptr<FieldDef> FieldDef::New() { upb_fielddef *f = upb_fielddef_new(&f); return reffed_ptr<FieldDef>(f, &f); } -inline FieldDef* FieldDef::Dup(const void* owner) const { - return upb_fielddef_dup(this, owner); -} inline const char* FieldDef::full_name() const { return upb_fielddef_fullname(this); } @@ -1886,9 +1831,6 @@ inline const OneofDef* MessageDef::FindOneofByName(const char* name, size_t len) const { return upb_msgdef_ntoo(this, name, len); } -inline MessageDef* MessageDef::Dup(const void *owner) const { - return upb_msgdef_dup(this, owner); -} inline void MessageDef::setmapentry(bool map_entry) { upb_msgdef_setmapentry(this, map_entry); } @@ -2058,9 +2000,6 @@ inline bool EnumDef::FindValueByName(const char* name, int32_t *num) const { inline const char* EnumDef::FindValueByNumber(int32_t num) const { return upb_enumdef_iton(this, num); } -inline EnumDef* EnumDef::Dup(const void* owner) const { - return upb_enumdef_dup(this, owner); -} inline EnumDef::Iterator::Iterator(const EnumDef* e) { upb_enum_begin(&iter_, e); |