Changes for PR comments.

tests/pb/test_encoder.cc

@@ -1,4 +1,5 @@
 
+#include "tests/test_util.h"
 #include "tests/upb_test.h"
 #include "upb/bindings/stdc++/string.h"
 #include "upb/descriptor/descriptor.upbdefs.h"
@@ -6,26 +7,6 @@
 #include "upb/pb/encoder.h"
 #include "upb/pb/glue.h"
 
-static char *upb_readfile(const char *filename, size_t *len) {
-  long size;
-  char *buf;
-  FILE *f = fopen(filename, "rb");
-  if(!f) return NULL;
-  if(fseek(f, 0, SEEK_END) != 0) goto error;
-  size = ftell(f);
-  if(size < 0) goto error;
-  if(fseek(f, 0, SEEK_SET) != 0) goto error;
-  buf = (char*)malloc(size + 1);
-  if(size && fread(buf, size, 1, f) != 1) goto error;
-  fclose(f);
-  if (len) *len = size;
-  return buf;
-
-error:
-  fclose(f);
-  return NULL;
-}
-
 std::string read_string(const char *filename) {
   size_t len;
   char *str = upb_readfile(filename, &len);

tests/test_def.c

@@ -3,6 +3,7 @@
 ** (like attempts to link defs that don't have required properties set).
 */
 
+#include "tests/test_util.h"
 #include "upb/def.h"
 #include "upb/pb/glue.h"
 #include "upb_test.h"
@@ -38,26 +39,6 @@ static void test_noreftracking() {
   upb_msgdef_unref(md, &md);
 }
 
-static char *upb_readfile(const char *filename, size_t *len) {
-  long size;
-  char *buf;
-  FILE *f = fopen(filename, "rb");
-  if(!f) return NULL;
-  if(fseek(f, 0, SEEK_END) != 0) goto error;
-  size = ftell(f);
-  if(size < 0) goto error;
-  if(fseek(f, 0, SEEK_SET) != 0) goto error;
-  buf = malloc(size + 1);
-  if(size && fread(buf, size, 1, f) != 1) goto error;
-  fclose(f);
-  if (len) *len = size;
-  return buf;
-
-error:
-  fclose(f);
-  return NULL;
-}
-
 static upb_symtab *load_test_proto(void *owner) {
   upb_symtab *s = upb_symtab_new(owner);
   upb_status status = UPB_STATUS_INIT;

tests/test_util.h

@@ -11,23 +11,26 @@
 #include "upb/env.h"
 #include "upb/sink.h"
 
+#ifdef __cplusplus
+
 upb::BufferHandle global_handle;
 
-// A convenience class for parser tests.  Provides some useful features:
-//
-//   - can support multiple calls to parse, to test the parser's handling
-//     of buffer seams.
-//
-//   - can output verbose output about each parse call when requested, for
-//     ease of debugging.
-//
-//   - can pass NULL for skipped regions of the input if requested.
-//
-//   - allocates and passes a separate buffer for each parsed region, to
-//     ensure that the parser is not erroneously overreading its buffer.
+/* A convenience class for parser tests.  Provides some useful features:
+ *
+ *   - can support multiple calls to parse, to test the parser's handling
+ *     of buffer seams.
+ *
+ *   - can output verbose output about each parse call when requested, for
+ *     ease of debugging.
+ *
+ *   - can pass NULL for skipped regions of the input if requested.
+ *
+ *   - allocates and passes a separate buffer for each parsed region, to
+ *     ensure that the parser is not erroneously overreading its buffer.
+ */
 class VerboseParserEnvironment {
  public:
-  // Pass verbose=true to print detailed diagnostics to stderr.
+  /* Pass verbose=true to print detailed diagnostics to stderr. */
   VerboseParserEnvironment(bool verbose) : verbose_(verbose) {
     env_.SetErrorFunction(&VerboseParserEnvironment::OnError, this);
   }
@@ -60,15 +63,16 @@ class VerboseParserEnvironment {
     skipped_with_null_ = false;
   }
 
-  // The user should call a series of:
-  //
-  // Reset(buf, len, may_skip);
-  // Start()
-  // ParseBuffer(X);
-  // ParseBuffer(Y);
-  // // Repeat ParseBuffer as desired, but last call should pass -1.
-  // ParseBuffer(-1);
-  // End();
+  /* The user should call a series of:
+   *
+   * Reset(buf, len, may_skip);
+   * Start()
+   * ParseBuffer(X);
+   * ParseBuffer(Y);
+   * // Repeat ParseBuffer as desired, but last call should pass -1.
+   * ParseBuffer(-1);
+   * End();
+   */
 
 
   bool Start() {
@@ -111,9 +115,9 @@ class VerboseParserEnvironment {
 
     ASSERT((size_t)bytes <= (len_ - ofs_));
 
-    // Copy buffer into a separate, temporary buffer.
-    // This is necessary to verify that the parser is not erroneously
-    // reading outside the specified bounds.
+    /* Copy buffer into a separate, temporary buffer.
+     * This is necessary to verify that the parser is not erroneously
+     * reading outside the specified bounds. */
     char *buf2 = NULL;
 
     if ((int)(ofs_ + bytes) <= skip_until_) {
@@ -125,7 +129,7 @@ class VerboseParserEnvironment {
     }
 
     if (buf2 == NULL && bytes == 0) {
-      // Decoders dont' support buf=NULL, bytes=0.
+      /* Decoders dont' support buf=NULL, bytes=0. */
       return true;
     }
 
@@ -189,18 +193,41 @@ class VerboseParserEnvironment {
   bool end_ok_;
   bool end_ok_set_;
 
-  // When our parse call returns a value greater than the number of bytes
-  // we passed in, the decoder is indicating to us that the next N bytes
-  // in the stream are not needed and can be skipped.  The user is allowed
-  // to pass a NULL buffer for those N bytes.
-  //
-  // skip_until_ is initially set to 0 if we should do this NULL-buffer
-  // skipping or -1 if we should not.  If we are open to doing NULL-buffer
-  // skipping and we get an opportunity to do it, we set skip_until to the
-  // stream offset where we can skip until.  The user can then test whether
-  // this happened by testing SkippedWithNull().
+  /* When our parse call returns a value greater than the number of bytes
+   * we passed in, the decoder is indicating to us that the next N bytes
+   * in the stream are not needed and can be skipped.  The user is allowed
+   * to pass a NULL buffer for those N bytes.
+   *
+   * skip_until_ is initially set to 0 if we should do this NULL-buffer
+   * skipping or -1 if we should not.  If we are open to doing NULL-buffer
+   * skipping and we get an opportunity to do it, we set skip_until to the
+   * stream offset where we can skip until.  The user can then test whether
+   * this happened by testing SkippedWithNull(). */
   int skip_until_;
   bool skipped_with_null_;
 };
 
+#endif  /* __cplusplus */
+
+UPB_INLINE char *upb_readfile(const char *filename, size_t *len) {
+  long size;
+  char *buf;
+  FILE *f = fopen(filename, "rb");
+  if(!f) return NULL;
+  if(fseek(f, 0, SEEK_END) != 0) goto error;
+  size = ftell(f);
+  if(size < 0) goto error;
+  if(fseek(f, 0, SEEK_SET) != 0) goto error;
+  buf = (char*)malloc(size + 1);
+  if(size && fread(buf, size, 1, f) != 1) goto error;
+  fclose(f);
+  if (len) *len = size;
+  buf[size] = '\0';
+  return buf;
+
+error:
+  fclose(f);
+  return NULL;
+}
+
 #endif

tools/dump_cinit.lua

@@ -560,7 +560,7 @@ local function dump_defs_for_type(format, defs, append)
 end
 
 local function make_children_map(file)
-  -- Maps full_name -> children.
+  -- Maps file:package() or msg:full_name() -> children.
   local map = {}
   for def in file:defs(upb.DEF_ANY) do
     local container = remove_name(def:full_name())

upb/def.c

@@ -74,21 +74,17 @@ upb_deftype_t upb_def_type(const upb_def *d) { return d->type; }
 const char *upb_def_fullname(const upb_def *d) { return d->fullname; }
 
 const char *upb_def_name(const upb_def *d) {
-  /* Return one past the last '.'. */
-  const char *ret = d->fullname;
-  const char *p = ret;
+  const char *p;
 
-  if (ret == NULL) {
+  if (d->fullname == NULL) {
     return NULL;
+  } else if ((p = strrchr(d->fullname, '.')) == NULL) {
+    /* No '.' in the name, return the full string. */
+    return d->fullname;
+  } else {
+    /* Return one past the last '.'. */
+    return p + 1;
   }
-
-  for (p = ret; *p; ++p) {
-    if (*p == '.') {
-      ret = p + 1;
-    }
-  }
-
-  return ret;
 }
 
 bool upb_def_setfullname(upb_def *def, const char *fullname, upb_status *s) {
@@ -1527,12 +1523,6 @@ bool upb_msgdef_addoneof(upb_msgdef *m, upb_oneofdef *o, const void *ref_donor,
   return true;
 }
 
-void upb_msgdef_setprimitiveshavepresence(upb_msgdef *m, bool have_presence) {
-  assert(!upb_msgdef_isfrozen(m));
-  assert(!m->base.file);
-  m->syntax = have_presence ? UPB_SYNTAX_PROTO2 : UPB_SYNTAX_PROTO3;
-}
-
 const upb_fielddef *upb_msgdef_itof(const upb_msgdef *m, uint32_t i) {
   upb_value val;
   return upb_inttable_lookup32(&m->itof, i, &val) ?
@@ -1898,17 +1888,26 @@ const upb_filedef *upb_filedef_dep(const upb_filedef *f, size_t i) {
 }
 
 bool upb_filedef_setname(upb_filedef *f, const char *name, upb_status *s) {
-  UPB_UNUSED(s);
+  name = upb_strdup(name);
+  if (!name) {
+    upb_status_seterrmsg(s, "Out of memory");
+    return false;
+  }
   free((void*)f->name);
-  f->name = upb_strdup(name);
+  f->name = name;
   return true;
 }
 
 bool upb_filedef_setpackage(upb_filedef *f, const char *package,
                             upb_status *s) {
   if (!upb_isident(package, strlen(package), true, s)) return false;
+  package = upb_strdup(package);
+  if (!package) {
+    upb_status_seterrmsg(s, "Out of memory");
+    return false;
+  }
   free((void*)f->package);
-  f->package = upb_strdup(package);
+  f->package = package;
   return true;
 }
 

upb/def.h

@@ -921,7 +921,6 @@ bool upb_msgdef_addfield(upb_msgdef *m, upb_fielddef *f, const void *ref_donor,
                          upb_status *s);
 bool upb_msgdef_addoneof(upb_msgdef *m, upb_oneofdef *o, const void *ref_donor,
                          upb_status *s);
-void upb_msgdef_setprimitiveshavepresence(upb_msgdef *m, bool have_presence);
 
 /* Field lookup in a couple of different variations:
  *   - itof = int to field
@@ -1153,6 +1152,7 @@ class upb::OneofDef {
    * by name once added to a message def. */
   const char* name() const;
   bool set_name(const char* name, Status* s);
+  bool set_name(const std::string& name, Status* s);
 
   /* Returns the number of fields currently defined in the oneof. */
   int field_count() const;
@@ -1306,7 +1306,8 @@ class upb::FileDef {
 
   /* Get/set name of the file (eg. "foo/bar.proto"). */
   const char* name() const;
-  bool set_name(const char* fullname, Status* s);
+  bool set_name(const char* name, Status* s);
+  bool set_name(const std::string& name, Status* s);
 
   /* Package name for definitions inside the file (eg. "foo.bar"). */
   const char* package() const;
@@ -1900,6 +1901,9 @@ inline const char* OneofDef::name() const {
 inline bool OneofDef::set_name(const char* name, Status* s) {
   return upb_oneofdef_setname(this, name, s);
 }
+inline bool OneofDef::set_name(const std::string& name, Status* s) {
+  return upb_oneofdef_setname(this, upb_safecstr(name), s);
+}
 inline int OneofDef::field_count() const {
   return upb_oneofdef_numfields(this);
 }
@@ -1979,6 +1983,9 @@ inline const char* FileDef::name() const {
 inline bool FileDef::set_name(const char* name, Status* s) {
   return upb_filedef_setname(this, name, s);
 }
+inline bool FileDef::set_name(const std::string& name, Status* s) {
+  return upb_filedef_setname(this, upb_safecstr(name), s);
+}
 inline const char* FileDef::package() const {
   return upb_filedef_package(this);
 }

upb/symtab.c

@@ -427,6 +427,11 @@ bool upb_symtab_addfile(upb_symtab *s, upb_filedef *file, upb_status *status) {
   n = upb_filedef_defcount(file);
   defs = malloc(sizeof(*defs) * n);
 
+  if (defs == NULL) {
+    upb_status_seterrmsg(status, "Out of memory");
+    return false;
+  }
+
   for (i = 0; i < n; i++) {
     defs[i] = upb_filedef_mutabledef(file, i);
   }