Get rid of upb_symtabtxn.

This type was nothing but a map of defs.
We can as easily just pass an array of defs
into upb_symtab_add().

tests/tests.c

@@ -19,7 +19,6 @@ static upb_symtab *load_test_proto() {
   }
   upb_status status = UPB_STATUS_INIT;
   upb_read_descriptor(s, descriptor, len, &status);
-  upb_status_print(&status, stderr);
   ASSERT(upb_ok(&status));
   upb_status_uninit(&status);
   free(descriptor);

upb/atomic.h

@@ -20,6 +20,7 @@
 #define UPB_ATOMIC_H_
 
 #include <stdbool.h>
+#include <assert.h>
 
 #ifdef __cplusplus
 extern "C" {
@@ -45,7 +46,7 @@ typedef struct {
 
 INLINE void upb_atomic_init(upb_atomic_t *a, int val) { a->v = val; }
 INLINE bool upb_atomic_ref(upb_atomic_t *a) { return a->v++ == 0; }
-INLINE bool upb_atomic_unref(upb_atomic_t *a) { return --a->v == 0; }
+INLINE bool upb_atomic_unref(upb_atomic_t *a) { assert(a->v > 0); return --a->v == 0; }
 INLINE int upb_atomic_read(upb_atomic_t *a) { return a->v; }
 INLINE bool upb_atomic_add(upb_atomic_t *a, int val) {
   a->v += val;

upb/def.c

@@ -485,46 +485,12 @@ upb_msg_iter upb_msg_next(upb_msgdef *m, upb_msg_iter iter) {
 }
 
 
-/* upb_symtabtxn **************************************************************/
+/* upb_symtab *****************************************************************/
 
 typedef struct {
   upb_def *def;
 } upb_symtab_ent;
 
-void upb_symtabtxn_init(upb_symtabtxn *t) {
-  upb_strtable_init(&t->deftab, 16, sizeof(upb_symtab_ent));
-}
-
-void upb_symtabtxn_uninit(upb_symtabtxn *txn) {
-  upb_strtable *t = &txn->deftab;
-  upb_strtable_iter i;
-  for(upb_strtable_begin(&i, t); !upb_strtable_done(&i); upb_strtable_next(&i)) {
-    const upb_symtab_ent *e = upb_strtable_iter_value(&i);
-    free(e->def);
-  }
-  upb_strtable_free(t);
-}
-
-bool upb_symtabtxn_add(upb_symtabtxn *t, upb_def *def) {
-  // TODO: check if already present.
-  upb_symtab_ent e = {def};
-  //fprintf(stderr, "txn Inserting: %p, ent: %p\n", e.def, &e);
-  upb_strtable_insert(&t->deftab, def->fqname, &e);
-  return true;
-}
-
-#if 0
-err:
-  // We need to free all defs from "tmptab."
-  upb_rwlock_unlock(&s->lock);
-  for(upb_symtab_ent *e = upb_strtable_begin(&tmptab); e;
-      e = upb_strtable_next(&tmptab, &e->e)) {
-    upb_def_unref(e->def);
-  }
-  upb_strtable_free(&tmptab);
-  return false;
-#endif
-
 // Given a symbol and the base symbol inside which it is defined, find the
 // symbol's definition in t.
 static upb_symtab_ent *upb_resolve(upb_strtable *t,
@@ -543,19 +509,6 @@ static upb_symtab_ent *upb_resolve(upb_strtable *t,
   }
 }
 
-void upb_symtabtxn_begin(upb_symtabtxn_iter *i, upb_symtabtxn *t) {
-  upb_strtable_begin(i, &t->deftab);
-}
-void upb_symtabtxn_next(upb_symtabtxn_iter *i) { upb_strtable_next(i); }
-bool upb_symtabtxn_done(upb_symtabtxn_iter *i) { return upb_strtable_done(i); }
-upb_def *upb_symtabtxn_iter_def(upb_symtabtxn_iter *i) {
-  const upb_symtab_ent *e = upb_strtable_iter_value(i);
-  return e->def;
-}
-
-
-/* upb_symtab public interface ************************************************/
-
 static void _upb_symtab_free(upb_strtable *t) {
   upb_strtable_iter i;
   upb_strtable_begin(&i, t);
@@ -641,7 +594,7 @@ upb_def *upb_symtab_resolve(upb_symtab *s, const char *base, const char *sym) {
 }
 
 bool upb_symtab_dfs(upb_def *def, upb_def **open_defs, int n,
-                    upb_symtabtxn *txn) {
+                    upb_strtable *addtab) {
   // This linear search makes the DFS O(n^2) in the length of the paths.
   // Could make this O(n) with a hash table, but n is small.
   for (int i = 0; i < n; i++) {
@@ -656,23 +609,37 @@ bool upb_symtab_dfs(upb_def *def, upb_def **open_defs, int n,
     for(i = upb_msg_begin(m); !upb_msg_done(i); i = upb_msg_next(m, i)) {
       upb_fielddef *f = upb_msg_iter_field(i);
       if (!upb_hasdef(f)) continue;
-      needcopy |= upb_symtab_dfs(f->def, open_defs, n, txn);
+      needcopy |= upb_symtab_dfs(f->def, open_defs, n, addtab);
     }
   }
 
-  bool replacing = (upb_strtable_lookup(&txn->deftab, m->base.fqname) != NULL);
+  bool replacing = (upb_strtable_lookup(addtab, m->base.fqname) != NULL);
   if (needcopy && !replacing) {
     upb_symtab_ent e = {upb_def_dup(def)};
     //fprintf(stderr, "Replacing def: %p\n", e.def);
-    upb_strtable_insert(&txn->deftab, def->fqname, &e);
+    upb_strtable_insert(addtab, def->fqname, &e);
     replacing = true;
   }
   return replacing;
 }
 
-bool upb_symtab_commit(upb_symtab *s, upb_symtabtxn *txn, upb_status *status) {
+bool upb_symtab_add(upb_symtab *s, upb_def **defs, int n, upb_status *status) {
   upb_rwlock_wrlock(&s->lock);
 
+  // Add all defs to a table for resolution.
+  upb_strtable addtab;
+  upb_strtable_init(&addtab, n, sizeof(upb_symtab_ent));
+  for (int i = 0; i < n; i++) {
+    upb_def *def = defs[i];
+    if (upb_strtable_lookup(&addtab, def->fqname)) {
+      upb_status_setf(status, UPB_ERROR,
+                      "Conflicting defs named '%s'", def->fqname);
+      upb_strtable_free(&addtab);
+      return false;
+    }
+    upb_strtable_insert(&addtab, def->fqname, &def);
+  }
+
   // All existing defs that can reach defs that are being replaced must
   // themselves be replaced with versions that will point to the new defs.
   // Do a DFS -- any path that finds a new def must replace all ancestors.
@@ -682,12 +649,11 @@ bool upb_symtab_commit(upb_symtab *s, upb_symtabtxn *txn, upb_status *status) {
   for(; !upb_strtable_done(&i); upb_strtable_next(&i)) {
     upb_def *open_defs[UPB_MAX_TYPE_DEPTH];
     const upb_symtab_ent *e = upb_strtable_iter_value(&i);
-    upb_symtab_dfs(e->def, open_defs, 0, txn);
+    upb_symtab_dfs(e->def, open_defs, 0, &addtab);
   }
 
   // Resolve all refs.
-  upb_strtable *txntab = &txn->deftab;
-  upb_strtable_begin(&i, txntab);
+  upb_strtable_begin(&i, &addtab);
   for(; !upb_strtable_done(&i); upb_strtable_next(&i)) {
     const upb_symtab_ent *e = upb_strtable_iter_value(&i);
     upb_msgdef *m = upb_dyncast_msgdef(e->def);
@@ -701,11 +667,11 @@ bool upb_symtab_commit(upb_symtab *s, upb_symtabtxn *txn, upb_status *status) {
       if(!upb_hasdef(f)) continue;  // No resolving necessary.
       const char *name = upb_downcast_unresolveddef(f->def)->name;
 
-      // Resolve from either the txntab (pending adds) or symtab (existing
+      // Resolve from either the addtab (pending adds) or symtab (existing
       // defs).  If both exist, prefer the pending add, because it will be
       // overwriting the existing def.
       upb_symtab_ent *found;
-      if(!(found = upb_resolve(txntab, base, name)) &&
+      if(!(found = upb_resolve(&addtab, base, name)) &&
          !(found = upb_resolve(symtab, base, name))) {
         upb_status_setf(status, UPB_ERROR, "could not resolve symbol '%s' "
                                            "in context '%s'", name, base);
@@ -727,7 +693,7 @@ bool upb_symtab_commit(upb_symtab *s, upb_symtabtxn *txn, upb_status *status) {
 
   // The defs in the transaction have been vetted, and can be moved to the
   // symtab without causing errors.
-  upb_strtable_begin(&i, txntab);
+  upb_strtable_begin(&i, &addtab);
   for(; !upb_strtable_done(&i); upb_strtable_next(&i)) {
     const upb_symtab_ent *tmptab_e = upb_strtable_iter_value(&i);
     upb_def_movetosymtab(tmptab_e->def, s);
@@ -742,7 +708,7 @@ bool upb_symtab_commit(upb_symtab *s, upb_symtabtxn *txn, upb_status *status) {
     }
   }
 
-  upb_strtable_clear(txntab);
+  upb_strtable_free(&addtab);
   upb_rwlock_unlock(&s->lock);
   upb_symtab_gc(s);
   return true;

upb/def.h

@@ -318,47 +318,6 @@ INLINE int32_t upb_enum_iter_number(upb_enum_iter iter) {
 }
 
 
-/* upb_symtabtxn **************************************************************/
-
-// A symbol table transaction is a map of defs that can be added to a symtab
-// in one single atomic operation that either succeeds or fails.  Mutable defs
-// can be added to this map (and perhaps removed, in the future).
-//
-// A symtabtxn is not thread-safe.
-
-typedef struct {
-  upb_strtable deftab;
-} upb_symtabtxn;
-
-void upb_symtabtxn_init(upb_symtabtxn *t);
-void upb_symtabtxn_uninit(upb_symtabtxn *t);
-
-// Adds a def to the symtab.  Caller passes a ref on the def to the symtabtxn.
-// The def's name must be set and there must not be any existing defs in the
-// symtabtxn with this name, otherwise false will be returned and no operation
-// will be performed (and the ref on the def will be released).
-bool upb_symtabtxn_add(upb_symtabtxn *t, upb_def *def);
-
-// Gets the def (if any) that is associated with this name in the symtab.
-// Caller does *not* inherit a ref on the def.
-upb_def *upb_symtabtxn_get(upb_symtabtxn *t, char *name);
-
-// Iterate over the defs that are part of the transaction.
-// The order is undefined.
-// The iterator is invalidated by upb_symtabtxn_add().
-//   upb_symtabtxn_iter i;
-//   for(i = upb_symtabtxn_begin(t); !upb_symtabtxn_done(t);
-//       i = upb_symtabtxn_next(t, i)) {
-//     upb_def *def = upb_symtabtxn_iter_def(i);
-//   }
-typedef upb_strtable_iter upb_symtabtxn_iter;
-
-void upb_symtabtxn_begin(upb_symtabtxn_iter* i, upb_symtabtxn *t);
-void upb_symtabtxn_next(upb_symtabtxn_iter *i);
-bool upb_symtabtxn_done(upb_symtabtxn_iter *i);
-upb_def *upb_symtabtxn_iter_def(upb_symtabtxn_iter *iter);
-
-
 /* upb_symtab *****************************************************************/
 
 // A SymbolTable is where upb_defs live.  It is empty when first constructed.
@@ -412,16 +371,12 @@ upb_def *upb_symtab_lookup(upb_symtab *s, const char *sym);
 // TODO: make return const
 upb_def **upb_symtab_getdefs(upb_symtab *s, int *n, upb_deftype_t type);
 
-// Adds a single upb_def into the symtab.  A ref on the def is passed to the
-// symtab.  If any references cannot be resolved, false is returned and the
-// symtab is unchanged.  The error (if any) is saved to status if non-NULL.
-bool upb_symtab_add(upb_symtab *s, upb_def *d, upb_status *status);
-
-// Adds the set of defs contained in the transaction to the symtab, clearing
-// the txn.  The entire operation either succeeds or fails.  If the operation
-// fails, the symtab is unchanged, false is returned, and status indicates
-// the error.
-bool upb_symtab_commit(upb_symtab *s, upb_symtabtxn *t, upb_status *status);
+// Adds the given defs to the symtab, resolving all symbols.  Only one def per
+// name may be in the list, but defs can replace existing defs in the symtab.
+// The entire operation either succeeds or fails.  If the operation fails, the
+// symtab is unchanged, false is returned, and status indicates the error.  A
+// ref on the defs is passed to the symtab iff the operation succeeds.
+bool upb_symtab_add(upb_symtab *s, upb_def **defs, int n, upb_status *status);
 
 // Frees defs that are no longer active in the symtab and are no longer
 // reachable.  Such defs are not freed when they are replaced in the symtab

upb/descriptor.c

@@ -51,10 +51,9 @@ static void upb_deflist_qualify(upb_deflist *l, char *str, int32_t start) {
 static upb_mhandlers *upb_msgdef_register_DescriptorProto(upb_handlers *h);
 static upb_mhandlers * upb_enumdef_register_EnumDescriptorProto(upb_handlers *h);
 
-void upb_descreader_init(upb_descreader *r, upb_symtabtxn *txn) {
+void upb_descreader_init(upb_descreader *r) {
   upb_deflist_init(&r->defs);
   upb_status_init(&r->status);
-  r->txn = txn;
   r->stack_len = 0;
   r->name = NULL;
   r->default_string = NULL;
@@ -71,6 +70,12 @@ void upb_descreader_uninit(upb_descreader *r) {
   }
 }
 
+upb_def **upb_descreader_getdefs(upb_descreader *r, int *n) {
+  *n = r->defs.len;
+  r->defs.len = 0;
+  return r->defs.defs;
+}
+
 static upb_msgdef *upb_descreader_top(upb_descreader *r) {
   if (r->stack_len <= 1) return NULL;
   int index = r->stack[r->stack_len-1].start - 1;
@@ -148,23 +153,8 @@ static upb_mhandlers *upb_descreader_register_FileDescriptorProto(
 #undef FNUM
 #undef FTYPE
 
-// Handlers for google.protobuf.FileDescriptorSet.
-static void upb_descreader_FileDescriptorSet_onendmsg(void *_r,
-                                                      upb_status *status) {
-  // Move all defs (which are now guaranteed to be fully-qualified) to the txn.
-  upb_descreader *r = _r;
-  if (upb_ok(status)) {
-    for (unsigned int i = 0; i < r->defs.len; i++) {
-      // TODO: check return for duplicate def.
-      upb_symtabtxn_add(r->txn, r->defs.defs[i]);
-    }
-    r->defs.len = 0;
-  }
-}
-
 static upb_mhandlers *upb_descreader_register_FileDescriptorSet(upb_handlers *h) {
   upb_mhandlers *m = upb_handlers_newmhandlers(h);
-  upb_mhandlers_setendmsg(m, upb_descreader_FileDescriptorSet_onendmsg);
 
 #define FNUM(field) GOOGLE_PROTOBUF_FILEDESCRIPTORSET_ ## field ## __FIELDNUM
 #define FTYPE(field) GOOGLE_PROTOBUF_FILEDESCRIPTORSET_ ## field ## __FIELDTYPE

upb/descriptor.h

@@ -36,7 +36,6 @@ typedef struct {
 
 typedef struct {
   upb_deflist defs;
-  upb_symtabtxn *txn;
   upb_descreader_frame stack[UPB_MAX_TYPE_DEPTH];
   int stack_len;
   upb_status status;
@@ -52,7 +51,7 @@ typedef struct {
 } upb_descreader;
 
 // Creates a new descriptor builder that will add defs to the given txn.
-void upb_descreader_init(upb_descreader *r, upb_symtabtxn *txn);
+void upb_descreader_init(upb_descreader *r);
 void upb_descreader_uninit(upb_descreader *r);
 
 // Registers handlers that will load descriptor data into a symtabtxn.
@@ -60,6 +59,13 @@ void upb_descreader_uninit(upb_descreader *r);
 // upb_msgdef_layout() called on them before adding to the txn.
 upb_mhandlers *upb_descreader_reghandlers(upb_handlers *h);
 
+// Gets the array of defs that have been parsed and removes them from the
+// descreader.  Ownership of the defs is passed to the caller, but the
+// ownership of the returned array is retained and is invalidated by any other
+// call into the descreader.  The defs will not have been resolved, and are
+// ready to be added to a symtab.
+upb_def **upb_descreader_getdefs(upb_descreader *r, int *n);
+
 #ifdef __cplusplus
 }  /* extern "C" */
 #endif

upb/pb/glue.c

@@ -66,19 +66,16 @@ void upb_read_descriptor(upb_symtab *symtab, const char *str, size_t len,
   upb_decoder_initforhandlers(&d, h);
   upb_handlers_unref(h);
   upb_descreader r;
-  upb_symtabtxn txn;
-  upb_symtabtxn_init(&txn);
-  upb_descreader_init(&r, &txn);
+  upb_descreader_init(&r);
   upb_decoder_reset(&d, upb_stringsrc_bytesrc(&strsrc), 0, UINT64_MAX, &r);
 
   upb_decoder_decode(&d, status);
+  int n;
+  upb_def **defs = upb_descreader_getdefs(&r, &n);
 
   // Set default accessors and layouts on all messages.
-  // for msgdef in symtabtxn:
-  upb_symtabtxn_iter i;
-  upb_symtabtxn_begin(&i, &txn);
-  for(; !upb_symtabtxn_done(&i); upb_symtabtxn_next(&i)) {
-    upb_def *def = upb_symtabtxn_iter_def(&i);
+  for(int i = 0; i < n; i++) {
+    upb_def *def = defs[i];
     upb_msgdef *md = upb_dyncast_msgdef(def);
     if (!md) return;
     // For field in msgdef:
@@ -90,9 +87,8 @@ void upb_read_descriptor(upb_symtab *symtab, const char *str, size_t len,
     upb_msgdef_layout(md);
   }
 
-  if (upb_ok(status)) upb_symtab_commit(symtab, &txn, status);
+  if (upb_ok(status)) upb_symtab_add(symtab, defs, n, status);
 
-  upb_symtabtxn_uninit(&txn);
   upb_descreader_uninit(&r);
   upb_stringsrc_uninit(&strsrc);
   upb_decoder_uninit(&d);

upb/table.h

@@ -107,12 +107,6 @@ void upb_inttable_insert(upb_inttable *t, uint32_t key, const void *val);
 // functions.
 void upb_strtable_insert(upb_strtable *t, const char *key, const void *val);
 void upb_inttable_compact(upb_inttable *t);
-INLINE void upb_strtable_clear(upb_strtable *t) {
-  // TODO: improve.
-  uint16_t entry_size = t->t.entry_size;
-  upb_strtable_free(t);
-  upb_strtable_init(t, 8, entry_size);
-}
 
 INLINE uint32_t _upb_inttable_bucket(upb_inttable *t, uint32_t k) {
   uint32_t bucket = k & t->t.mask;  // Identity hash for ints.