From 8823fa6069452c86325b4d5a6065a0d4fd67f1a2 Mon Sep 17 00:00:00 2001 From: Josh Haberman Date: Wed, 6 Apr 2016 16:12:26 -0700 Subject: Refactored upb_def_freeze() a bit per PR comments. --- tests/test_def.c | 1 + upb/def.c | 39 +++++++++++++++++++-------------------- upb/def.h | 3 +-- upb/refcounted.c | 5 ++++- upb/symtab.c | 23 ++++++++++++++++++----- 5 files changed, 43 insertions(+), 28 deletions(-) diff --git a/tests/test_def.c b/tests/test_def.c index cc766e0..de3bcb2 100644 --- a/tests/test_def.c +++ b/tests/test_def.c @@ -402,6 +402,7 @@ static void test_mapentry_check() { /* Should not have succeeded: non-repeated field pointing to a MapEntry. */ ASSERT(!upb_ok(&s)); + upb_status_clear(&s); upb_fielddef_setlabel(f, UPB_LABEL_REPEATED); upb_symtab_add(symtab, defs, 2, NULL, &s); ASSERT(upb_ok(&s)); diff --git a/upb/def.c b/upb/def.c index 2fdbf01..27484a3 100644 --- a/upb/def.c +++ b/upb/def.c @@ -335,17 +335,13 @@ static bool assign_msg_indices(upb_msgdef *m, upb_status *s) { return true; } -bool upb_def_freeze2(upb_refcounted *const* objs, size_t defs_n, - size_t freeze_n, upb_status *s) { +bool _upb_def_validate(upb_def *const*defs, size_t n, upb_status *s) { size_t i; - size_t maxdepth; - bool ret; - upb_status_clear(s); /* First perform validation, in two passes so we can check that we have a * transitive closure without needing to search. */ - for (i = 0; i < defs_n; i++) { - upb_def *def = (upb_def*)objs[i]; + for (i = 0; i < n; i++) { + upb_def *def = defs[i]; if (upb_def_isfrozen(def)) { /* Could relax this requirement if it's annoying. */ upb_status_seterrmsg(s, "def is already frozen"); @@ -365,8 +361,8 @@ bool upb_def_freeze2(upb_refcounted *const* objs, size_t defs_n, /* Second pass of validation. Also assign selector bases and indexes, and * compact tables. */ - for (i = 0; i < defs_n; i++) { - upb_def *def = (upb_def*)objs[i]; + for (i = 0; i < n; i++) { + upb_def *def = defs[i]; upb_msgdef *m = upb_dyncast_msgdef_mutable(def); upb_enumdef *e = upb_dyncast_enumdef_mutable(def); if (m) { @@ -379,18 +375,11 @@ bool upb_def_freeze2(upb_refcounted *const* objs, size_t defs_n, } } - /* Def graph contains FieldDefs between each MessageDef, so double the - * limit. */ - maxdepth = UPB_MAX_MESSAGE_DEPTH * 2; - - /* Validation all passed; freeze the objects. */ - ret = upb_refcounted_freeze(objs, freeze_n, s, maxdepth); - assert(!(s && ret != upb_ok(s))); - return ret; + return true; err: - for (i = 0; i < defs_n; i++) { - upb_def *def = (upb_def*)objs[i]; + for (i = 0; i < n; i++) { + upb_def *def = defs[i]; def->came_from_user = false; } assert(!(s && upb_ok(s))); @@ -398,7 +387,17 @@ err: } bool upb_def_freeze(upb_def *const* defs, size_t n, upb_status *s) { - return upb_def_freeze2((upb_refcounted *const*)defs, n, n, s); + /* Def graph contains FieldDefs between each MessageDef, so double the + * limit. */ + const size_t maxdepth = UPB_MAX_MESSAGE_DEPTH * 2; + + if (!_upb_def_validate(defs, n, s)) { + return false; + } + + + /* Validation all passed; freeze the objects. */ + return upb_refcounted_freeze((upb_refcounted *const*)defs, n, s, maxdepth); } diff --git a/upb/def.h b/upb/def.h index 0fc6f41..d4808a6 100644 --- a/upb/def.h +++ b/upb/def.h @@ -137,8 +137,7 @@ bool upb_def_setfullname(upb_def *def, const char *fullname, upb_status *s); bool upb_def_freeze(upb_def *const *defs, size_t n, upb_status *s); /* Temporary API: for internal use only. */ -bool upb_def_freeze2(upb_refcounted *const *objs, size_t defs_n, - size_t freeze_n, upb_status *s); +bool _upb_def_validate(upb_def *const*defs, size_t n, upb_status *s); UPB_END_EXTERN_C diff --git a/upb/refcounted.c b/upb/refcounted.c index c8162f0..81ac798 100644 --- a/upb/refcounted.c +++ b/upb/refcounted.c @@ -838,8 +838,11 @@ void upb_refcounted_checkref(const upb_refcounted *r, const void *owner) { bool upb_refcounted_freeze(upb_refcounted *const*roots, int n, upb_status *s, int maxdepth) { int i; + bool ret; for (i = 0; i < n; i++) { assert(!roots[i]->is_frozen); } - return freeze(roots, n, s, maxdepth); + ret = freeze(roots, n, s, maxdepth); + assert(!s || ret == upb_ok(s)); + return ret; } diff --git a/upb/symtab.c b/upb/symtab.c index 83f549e..b694104 100644 --- a/upb/symtab.c +++ b/upb/symtab.c @@ -202,6 +202,7 @@ static bool symtab_add(upb_symtab *s, upb_def *const*defs, size_t n, size_t freeze_n; upb_strtable_iter iter; upb_refcounted **add_objs = NULL; + upb_def **add_defs = NULL; size_t add_objs_size; upb_strtable addtab; upb_inttable seen; @@ -347,26 +348,38 @@ static bool symtab_add(upb_symtab *s, upb_def *const*defs, size_t n, } } - /* We need an array of the defs in addtab, for passing to upb_def_freeze. */ + /* We need an array of the defs in addtab, for passing to + * upb_refcounted_freeze(). */ add_objs_size = upb_strtable_count(&addtab); if (freeze_also) { add_objs_size++; } - add_objs = malloc(sizeof(void*) * add_objs_size); - if (add_objs == NULL) goto oom_err; + add_defs = malloc(sizeof(void*) * add_objs_size); + if (add_defs == NULL) goto oom_err; upb_strtable_begin(&iter, &addtab); for (add_n = 0; !upb_strtable_done(&iter); upb_strtable_next(&iter)) { - add_objs[add_n++] = upb_value_getptr(upb_strtable_iter_value(&iter)); + add_defs[add_n++] = upb_value_getptr(upb_strtable_iter_value(&iter)); } + /* Validate defs. */ + if (!_upb_def_validate(add_defs, add_n, status)) { + goto err; + } + + /* Cheat a little and give the array a new type. + * This is probably undefined behavior, but this code will be deleted soon. */ + add_objs = (upb_refcounted**)add_defs; + freeze_n = add_n; if (freeze_also) { add_objs[freeze_n++] = freeze_also; } - if (!upb_def_freeze2(add_objs, add_n, freeze_n, status)) + if (!upb_refcounted_freeze(add_objs, freeze_n, status, + UPB_MAX_MESSAGE_DEPTH * 2)) { goto err; + } /* This must be delayed until all errors have been detected, since error * recovery code uses this table to cleanup defs. */ -- cgit v1.2.3