Refactored upb_def_freeze() a bit per PR comments.

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));

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);
 }
 
 

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
 

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;
 }

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. */