From 47da2afd52b0f108085439e3dc8ad5236809fbae Mon Sep 17 00:00:00 2001 From: Josh Haberman Date: Sat, 21 Jan 2017 10:47:58 -0800 Subject: Make upb::SymbolTable no longer reference-counted. This transitions it from shared ownership to unique ownership. --- upb/bindings/lua/def.c | 57 ++++++++++++++++++-------------------------------- upb/bindings/lua/upb.h | 2 +- upb/def.c | 22 ++----------------- upb/def.h | 26 ++++++++--------------- upb/msg.c | 2 -- 5 files changed, 32 insertions(+), 77 deletions(-) (limited to 'upb') diff --git a/upb/bindings/lua/def.c b/upb/bindings/lua/def.c index 638abb6..5133831 100644 --- a/upb/bindings/lua/def.c +++ b/upb/bindings/lua/def.c @@ -1090,50 +1090,35 @@ static const struct luaL_Reg lupb_filedef_m[] = { /* lupb_symtab ****************************************************************/ -/* Inherits a ref on the symtab. - * Checks that narg is a proper lupb_symtab object. If it is, leaves its - * metatable on the stack for cache lookups/updates. */ -const upb_symtab *lupb_symtab_check(lua_State *L, int narg) { - return lupb_refcounted_check(L, narg, LUPB_SYMTAB); -} - -static upb_symtab *lupb_symtab_checkmutable(lua_State *L, int narg) { - const upb_symtab *s = lupb_symtab_check(L, narg); - if (upb_symtab_isfrozen(s)) - luaL_error(L, "not allowed on frozen value"); - return (upb_symtab*)s; -} - -void lupb_symtab_pushwrapper(lua_State *L, const upb_symtab *s, - const void *ref_donor) { - lupb_refcounted_pushwrapper(L, upb_symtab_upcast(s), LUPB_SYMTAB, ref_donor, - sizeof(void *)); -} +typedef struct { + upb_symtab *symtab; +} lupb_symtab; -void lupb_symtab_pushnewrapper(lua_State *L, const upb_symtab *s, - const void *ref_donor) { - lupb_refcounted_pushnewrapper(L, upb_symtab_upcast(s), LUPB_SYMTAB, - ref_donor); +upb_symtab *lupb_symtab_check(lua_State *L, int narg) { + lupb_symtab *lsymtab = luaL_checkudata(L, narg, LUPB_SYMTAB); + if (!lsymtab->symtab) { + luaL_error(L, "called into dead object"); + } + return lsymtab->symtab; } static int lupb_symtab_new(lua_State *L) { - upb_symtab *s = upb_symtab_new(&s); - lupb_symtab_pushnewrapper(L, s, &s); + lupb_symtab *lsymtab = lua_newuserdata(L, sizeof(*lsymtab)); + lsymtab->symtab = upb_symtab_new(); + luaL_getmetatable(L, LUPB_SYMTAB); + lua_setmetatable(L, -2); return 1; } -static int lupb_symtab_freeze(lua_State *L) { - upb_symtab_freeze(lupb_symtab_checkmutable(L, 1)); +static int lupb_symtab_gc(lua_State *L) { + lupb_symtab *lsymtab = luaL_checkudata(L, 1, LUPB_SYMTAB); + upb_symtab_free(lsymtab->symtab); + lsymtab->symtab = NULL; return 0; } -static int lupb_symtab_isfrozen(lua_State *L) { - lua_pushboolean(L, upb_symtab_isfrozen(lupb_symtab_check(L, 1))); - return 1; -} - static int lupb_symtab_add(lua_State *L) { - upb_symtab *s = lupb_symtab_checkmutable(L, 1); + upb_symtab *s = lupb_symtab_check(L, 1); int n; upb_def **defs; @@ -1161,7 +1146,7 @@ static int lupb_symtab_add(lua_State *L) { } static int lupb_symtab_addfile(lua_State *L) { - upb_symtab *s = lupb_symtab_checkmutable(L, 1); + upb_symtab *s = lupb_symtab_check(L, 1); upb_filedef *f = lupb_filedef_checkmutable(L, 2); CHK(upb_symtab_addfile(s, f, &status)); return 0; @@ -1201,14 +1186,12 @@ static const struct luaL_Reg lupb_symtab_m[] = { {"add", lupb_symtab_add}, {"add_file", lupb_symtab_addfile}, {"defs", lupb_symtab_defs}, - {"freeze", lupb_symtab_freeze}, - {"is_frozen", lupb_symtab_isfrozen}, {"lookup", lupb_symtab_lookup}, {NULL, NULL} }; static const struct luaL_Reg lupb_symtab_mm[] = { - {"__gc", lupb_refcounted_gc}, + {"__gc", lupb_symtab_gc}, {NULL, NULL} }; diff --git a/upb/bindings/lua/upb.h b/upb/bindings/lua/upb.h index 84212c5..88a201c 100644 --- a/upb/bindings/lua/upb.h +++ b/upb/bindings/lua/upb.h @@ -101,7 +101,7 @@ void *lupb_refcounted_check(lua_State *L, int narg, const char *type); const upb_msgdef *lupb_msgdef_check(lua_State *L, int narg); const upb_enumdef *lupb_enumdef_check(lua_State *L, int narg); const upb_fielddef *lupb_fielddef_check(lua_State *L, int narg); -const upb_symtab *lupb_symtab_check(lua_State *L, int narg); +upb_symtab *lupb_symtab_check(lua_State *L, int narg); void lupb_refcounted_pushnewrapper(lua_State *L, const upb_refcounted *obj, const char *type, const void *ref_donor); diff --git a/upb/def.c b/upb/def.c index 45cd362..39d8c08 100644 --- a/upb/def.c +++ b/upb/def.c @@ -2142,8 +2142,7 @@ bool upb_filedef_adddep(upb_filedef *f, const upb_filedef *dep) { } } -static void upb_symtab_free(upb_refcounted *r) { - upb_symtab *s = (upb_symtab*)r; +void upb_symtab_free(upb_symtab *s) { upb_strtable_iter i; upb_strtable_begin(&i, &s->symtab); for (; !upb_strtable_done(&i); upb_strtable_next(&i)) { @@ -2154,32 +2153,16 @@ static void upb_symtab_free(upb_refcounted *r) { upb_gfree(s); } -upb_symtab *upb_symtab_new(const void *owner) { - static const struct upb_refcounted_vtbl vtbl = {NULL, &upb_symtab_free}; - +upb_symtab *upb_symtab_new() { upb_symtab *s = upb_gmalloc(sizeof(*s)); if (!s) { return NULL; } - upb_refcounted_init(upb_symtab_upcast_mutable(s), &vtbl, owner); upb_strtable_init(&s->symtab, UPB_CTYPE_PTR); return s; } -void upb_symtab_freeze(upb_symtab *s) { - upb_refcounted *r; - bool ok; - - UPB_ASSERT(!upb_symtab_isfrozen(s)); - r = upb_symtab_upcast_mutable(s); - /* The symtab does not take ref2's (see refcounted.h) on the defs, because - * defs cannot refer back to the table and therefore cannot create cycles. So - * 0 will suffice for maxdepth here. */ - ok = upb_refcounted_freeze(&r, 1, NULL, 0); - UPB_ASSERT(ok); -} - const upb_def *upb_symtab_lookup(const upb_symtab *s, const char *sym) { upb_value v; upb_def *ret = upb_strtable_lookup(&s->symtab, sym, &v) ? @@ -2352,7 +2335,6 @@ static bool symtab_add(upb_symtab *s, upb_def *const*defs, size_t n, return true; } - UPB_ASSERT(!upb_symtab_isfrozen(s)); if (!upb_strtable_init(&addtab, UPB_CTYPE_PTR)) { upb_status_seterrmsg(status, "out of memory"); return false; diff --git a/upb/def.h b/upb/def.h index b2c42a6..b99dcb0 100644 --- a/upb/def.h +++ b/upb/def.h @@ -44,8 +44,7 @@ UPB_DECLARE_DERIVED_TYPE(upb::OneofDef, upb::RefCounted, upb_oneofdef, upb_refcounted) UPB_DECLARE_DERIVED_TYPE(upb::FileDef, upb::RefCounted, upb_filedef, upb_refcounted) -UPB_DECLARE_DERIVED_TYPE(upb::SymbolTable, upb::RefCounted, - upb_symtab, upb_refcounted) +UPB_DECLARE_TYPE(upb::SymbolTable, upb_symtab) /* The maximum message depth that the type graph can have. This is a resource @@ -1434,10 +1433,8 @@ class upb::SymbolTable { public: /* Returns a new symbol table with a single ref owned by "owner." * Returns NULL if memory allocation failed. */ - static reffed_ptr New(); - - /* Include RefCounted base methods. */ - UPB_REFCOUNTED_CPPMETHODS + static SymbolTable* New(); + static void Free(upb::SymbolTable* table); /* For all lookup functions, the returned pointer is not owned by the * caller; it may be invalidated by any non-const call or unref of the @@ -1527,11 +1524,8 @@ UPB_BEGIN_EXTERN_C /* Native C API. */ -/* Include refcounted methods like upb_symtab_ref(). */ -UPB_REFCOUNTED_CMETHODS(upb_symtab, upb_symtab_upcast) - -upb_symtab *upb_symtab_new(const void *owner); -void upb_symtab_freeze(upb_symtab *s); +upb_symtab *upb_symtab_new(); +void upb_symtab_free(upb_symtab* s); const upb_def *upb_symtab_resolve(const upb_symtab *s, const char *base, const char *sym); const upb_def *upb_symtab_lookup(const upb_symtab *s, const char *sym); @@ -1562,13 +1556,11 @@ UPB_END_EXTERN_C #ifdef __cplusplus /* C++ inline wrappers. */ namespace upb { -inline reffed_ptr SymbolTable::New() { - upb_symtab *s = upb_symtab_new(&s); - return reffed_ptr(s, &s); +inline SymbolTable* SymbolTable::New() { + return upb_symtab_new(); } - -inline void SymbolTable::Freeze() { - return upb_symtab_freeze(this); +inline void SymbolTable::Free(SymbolTable* s) { + upb_symtab_free(s); } inline const Def *SymbolTable::Resolve(const char *base, const char *sym) const { diff --git a/upb/msg.c b/upb/msg.c index 517b814..5fa9bc8 100644 --- a/upb/msg.c +++ b/upb/msg.c @@ -350,7 +350,6 @@ upb_msgfactory *upb_msgfactory_new(const upb_symtab *symtab) { upb_msgfactory *ret = upb_gmalloc(sizeof(*ret)); ret->symtab = symtab; - upb_symtab_ref(ret->symtab, &ret->symtab); upb_inttable_init(&ret->layouts, UPB_CTYPE_PTR); upb_inttable_init(&ret->mergehandlers, UPB_CTYPE_CONSTPTR); @@ -373,7 +372,6 @@ void upb_msgfactory_free(upb_msgfactory *f) { upb_inttable_uninit(&f->layouts); upb_inttable_uninit(&f->mergehandlers); - upb_symtab_unref(f->symtab, &f->symtab); upb_gfree(f); } -- cgit v1.2.3 From 629b4ce621abe41823b883be95805e9349e355ba Mon Sep 17 00:00:00 2001 From: Josh Haberman Date: Sat, 21 Jan 2017 11:19:46 -0800 Subject: Ripped out complicated and unused code for replacing defs in a symtab. Also hid the dup() functions. We can't quite delete them yet because our current approach for extensions depends on duplicating defs. --- tests/test_def.c | 68 ++++----------------- upb/def.c | 183 +++++-------------------------------------------------- upb/def.h | 69 ++------------------- 3 files changed, 33 insertions(+), 287 deletions(-) (limited to 'upb') 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(); diff --git a/upb/def.c b/upb/def.c index 39d8c08..1881dfa 100644 --- a/upb/def.c +++ b/upb/def.c @@ -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); diff --git a/upb/def.h b/upb/def.h index b99dcb0..9194a40 100644 --- a/upb/def.h +++ b/upb/def.h @@ -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 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 { 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& 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::New() { upb_fielddef *f = upb_fielddef_new(&f); return reffed_ptr(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); -- cgit v1.2.3 From 693b841ec6e2bdb91920b7ea35a70ca30b416741 Mon Sep 17 00:00:00 2001 From: Josh Haberman Date: Mon, 23 Jan 2017 16:06:26 -0800 Subject: Removed all code for adding extensions to upb_symtab. This means extensions can't be used until we implement the replacement APIs for accessing extensions from a symtab. --- tests/bindings/lua/test_upb.lua | 36 ---------- tests/test_def.c | 1 + upb/def.c | 144 ++-------------------------------------- 3 files changed, 5 insertions(+), 176 deletions(-) (limited to 'upb') diff --git a/tests/bindings/lua/test_upb.lua b/tests/bindings/lua/test_upb.lua index 261328d..1dc0717 100644 --- a/tests/bindings/lua/test_upb.lua +++ b/tests/bindings/lua/test_upb.lua @@ -403,42 +403,6 @@ function test_symtab() assert_equal(msgdef3:field("field5"):subdef(), msgdef2) end -function test_symtab_add_extension() - -- Adding an extension at the same time as the extendee. - local symtab = upb.SymbolTable{ - upb.MessageDef{full_name = "M1"}, - upb.FieldDef{name = "extension1", is_extension = true, number = 1, - type = upb.TYPE_INT32, containing_type_name = "M1"} - } - - local m1 = symtab:lookup("M1") - assert_not_nil(m1) - assert_equal(1, #m1) - - local f1 = m1:field("extension1") - assert_not_nil(f1) - assert_true(f1:is_extension()) - assert_true(f1:is_frozen()) - assert_equal(1, f1:number()) - - -- Adding an extension to an existing extendee. - symtab:add{ - upb.FieldDef{name = "extension2", is_extension = true, number = 2, - type = upb.TYPE_INT32, containing_type_name = "M1"} - } - - local m1_2 = symtab:lookup("M1") - assert_not_nil(m1_2) - assert_true(m1 ~= m1_2) - assert_equal(2, #m1_2) - - local f2 = m1_2:field("extension2") - assert_not_nil(f2) - assert_true(f2:is_extension()) - assert_true(f2:is_frozen()) - assert_equal(2, f2:number()) -end - function test_numeric_array() local function test_for_numeric_type(upb_type, val, too_big, too_small, bad3) local array = upb.Array(upb_type) diff --git a/tests/test_def.c b/tests/test_def.c index 9859ca0..76914c7 100644 --- a/tests/test_def.c +++ b/tests/test_def.c @@ -220,6 +220,7 @@ static void test_replacement_fails() { newdefs[1] = upb_msgdef_upcast_mutable(m2); ok = upb_symtab_add(s, newdefs, 2, &s, &status); ASSERT(ok == false); + upb_status_clear(&status); /* Adding just one is ok. */ ASSERT_STATUS(upb_symtab_add(s, newdefs, 1, &s, &status), &status); diff --git a/upb/def.c b/upb/def.c index 1881dfa..c69e506 100644 --- a/upb/def.c +++ b/upb/def.c @@ -711,43 +711,6 @@ upb_fielddef *upb_fielddef_new(const void *o) { return f; } -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; - upb_fielddef_settype(newf, upb_fielddef_type(f)); - upb_fielddef_setlabel(newf, upb_fielddef_label(f)); - upb_fielddef_setnumber(newf, upb_fielddef_number(f), NULL); - upb_fielddef_setname(newf, upb_fielddef_name(f), NULL); - if (f->default_is_string && f->defaultval.bytes) { - str_t *s = f->defaultval.bytes; - upb_fielddef_setdefaultstr(newf, s->str, s->len, NULL); - } else { - newf->default_is_string = f->default_is_string; - newf->defaultval = f->defaultval; - } - - if (f->subdef_is_symbolic) { - srcname = f->sub.name; /* Might be NULL. */ - } else { - srcname = f->sub.def ? upb_def_fullname(f->sub.def) : NULL; - } - if (srcname) { - char *newname = upb_gmalloc(strlen(f->sub.def->fullname) + 2); - if (!newname) { - upb_fielddef_unref(newf, owner); - return NULL; - } - strcpy(newname, "."); - strcat(newname, f->sub.def->fullname); - upb_fielddef_setsubdefname(newf, newname, NULL); - upb_gfree(newname); - } - - return newf; -} - bool upb_fielddef_typeisset(const upb_fielddef *f) { return f->type_is_set_; } @@ -1427,44 +1390,6 @@ err2: return NULL; } -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; - - upb_msgdef *newm = upb_msgdef_new(owner); - if (!newm) return NULL; - ok = upb_def_setfullname(upb_msgdef_upcast_mutable(newm), - upb_def_fullname(upb_msgdef_upcast(m)), - NULL); - newm->map_entry = m->map_entry; - newm->syntax = m->syntax; - UPB_ASSERT(ok); - for(upb_msg_field_begin(&i, m); - !upb_msg_field_done(&i); - upb_msg_field_next(&i)) { - upb_fielddef *f = upb_fielddef_dup(upb_msg_iter_field(&i), &f); - /* Fields in oneofs are dup'd below. */ - if (upb_fielddef_containingoneof(f)) continue; - if (!f || !upb_msgdef_addfield(newm, f, &f, NULL)) { - upb_msgdef_unref(newm, owner); - return NULL; - } - } - for(upb_msg_oneof_begin(&o, m); - !upb_msg_oneof_done(&o); - upb_msg_oneof_next(&o)) { - upb_oneofdef *f = upb_oneofdef_dup(upb_msg_iter_oneof(&o), &f); - if (!f || !upb_msgdef_addoneof(newm, f, &f, NULL)) { - upb_msgdef_unref(newm, owner); - return NULL; - } - } - return newm; -} - bool upb_msgdef_freeze(upb_msgdef *m, upb_status *status) { upb_def *d = upb_msgdef_upcast_mutable(m); return upb_def_freeze(&d, 1, status); @@ -1765,24 +1690,6 @@ err2: return NULL; } -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); - if (!newo) return NULL; - ok = upb_oneofdef_setname(newo, upb_oneofdef_name(o), NULL); - UPB_ASSERT(ok); - for (upb_oneof_begin(&i, o); !upb_oneof_done(&i); upb_oneof_next(&i)) { - upb_fielddef *f = upb_fielddef_dup(upb_oneof_iter_field(&i), &f); - if (!f || !upb_oneofdef_addfield(newo, f, &f, NULL)) { - upb_oneofdef_unref(newo, owner); - return NULL; - } - } - return newo; -} - const char *upb_oneofdef_name(const upb_oneofdef *o) { return o->name; } bool upb_oneofdef_setname(upb_oneofdef *o, const char *name, upb_status *s) { @@ -2241,57 +2148,14 @@ static bool symtab_add(upb_symtab *s, upb_def *const*defs, size_t n, 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; - } - } - - /* Add standalone fielddefs (ie. extensions) to the appropriate messages. - * If the appropriate message only exists in the existing symtab, duplicate - * it so we have a mutable copy we can add the fields to. */ - for (i = 0; i < n; i++) { - upb_def *def = defs[i]; - upb_fielddef *f = upb_dyncast_fielddef_mutable(def); - const char *msgname; - upb_value v; - upb_msgdef *m; - - if (!f) continue; - msgname = upb_fielddef_containingtypename(f); - /* We validated this earlier in this function. */ - UPB_ASSERT(msgname); - - /* If the extendee name is absolutely qualified, move past the initial ".". - * TODO(haberman): it is not obvious what it would mean if this was not - * absolutely qualified. */ - if (msgname[0] == '.') { - msgname++; - } - - if (upb_strtable_lookup(&addtab, msgname, &v)) { - /* Extendee is in the set of defs the user asked us to add. */ - m = upb_value_getptr(v); - } else { - /* Need to find and dup the extendee from the existing symtab. */ - const upb_msgdef *frozen_m = upb_symtab_lookupmsg(s, msgname); - if (!frozen_m) { - upb_status_seterrf(status, - "Tried to extend message %s that does not exist " - "in this SymbolTable.", - msgname); - goto err; - } - m = upb_msgdef_dup(frozen_m, s); - if (!m) goto oom_err; - if (!upb_strtable_insert(&addtab, msgname, upb_value_ptr(m))) { - upb_msgdef_unref(m, s); - goto oom_err; - } + upb_def_donateref(def, ref_donor, s); } - if (!upb_msgdef_addfield(m, f, ref_donor, status)) { + if (upb_dyncast_fielddef_mutable(def)) { + /* TODO(haberman): allow adding extensions attached to files. */ + upb_status_seterrf(status, "Can't add extensions to symtab.\n"); goto err; } } -- cgit v1.2.3 From 512130adf13809cb857c9a208d728514e5ea31fd Mon Sep 17 00:00:00 2001 From: Josh Haberman Date: Mon, 23 Jan 2017 16:33:33 -0800 Subject: Remove another bit of obsolete code. --- upb/def.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) (limited to 'upb') diff --git a/upb/def.c b/upb/def.c index c69e506..080cd92 100644 --- a/upb/def.c +++ b/upb/def.c @@ -2237,15 +2237,9 @@ static bool symtab_add(upb_symtab *s, upb_def *const*defs, size_t n, for (i = 0; i < add_n; i++) { upb_def *def = (upb_def*)add_objs[i]; const char *name = upb_def_fullname(def); - upb_value v; bool success; - - if (upb_strtable_remove(&s->symtab, name, &v)) { - const upb_def *def = upb_value_getptr(v); - upb_def_unref(def, s); - } success = upb_strtable_insert(&s->symtab, name, upb_value_ptr(def)); - UPB_ASSERT(success == true); + UPB_ASSERT(success); } upb_gfree(add_defs); return true; -- cgit v1.2.3 From 5aa01b46e41dcdbbd2dad6d1b7ecce9a60b8ce7a Mon Sep 17 00:00:00 2001 From: Josh Haberman Date: Mon, 23 Jan 2017 16:56:39 -0800 Subject: A couple more fixes. --- tests/test_def.c | 2 +- upb/def.c | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) (limited to 'upb') diff --git a/tests/test_def.c b/tests/test_def.c index 76914c7..d82fddb 100644 --- a/tests/test_def.c +++ b/tests/test_def.c @@ -164,7 +164,7 @@ static void test_symbol_resolution() { static void test_fielddef_unref() { bool ok; - upb_symtab *s = load_test_proto(&s); + upb_symtab *s = load_test_proto(); const upb_msgdef *md = upb_symtab_lookupmsg(s, "A"); const upb_fielddef *f = upb_msgdef_itof(md, 1); upb_fielddef_ref(f, &f); diff --git a/upb/def.c b/upb/def.c index 080cd92..5fe9e14 100644 --- a/upb/def.c +++ b/upb/def.c @@ -378,13 +378,14 @@ bool _upb_def_validate(upb_def *const*defs, size_t n, upb_status *s) { } else if (def->type == UPB_DEF_FIELD) { upb_status_seterrmsg(s, "standalone fielddefs can not be frozen"); goto err; - } else if (def->type == UPB_DEF_ENUM) { - if (!upb_validate_enumdef(upb_dyncast_enumdef(def), s)) { - goto err; - } } else { /* Set now to detect transitive closure in the second pass. */ def->came_from_user = true; + + if (def->type == UPB_DEF_ENUM && + !upb_validate_enumdef(upb_dyncast_enumdef(def), s)) { + goto err; + } } } -- cgit v1.2.3