Fleshed out fielddef default functionality.

Fixes unit test submitted by Hunter Morris (thanks!).

upb/bytestream.c

@@ -21,6 +21,24 @@ char *upb_strref_dup(const struct _upb_strref *r) {
   return ret;
 }
 
+upb_strref *upb_strref_new(const char *str) {
+  return upb_strref_newl(str, strlen(str));
+}
+
+upb_strref *upb_strref_newl(const void *str, size_t len) {
+  upb_strref *s = malloc(sizeof(*s));
+  s->bytesrc = NULL;
+  s->ptr = malloc(len);
+  memcpy((void*)s->ptr, str, len);
+  return s;
+}
+
+void upb_strref_free(upb_strref *ref) {
+  if (!ref) return;
+  free((char*)ref->ptr);
+  free(ref);
+}
+
 void upb_bytesink_init(upb_bytesink *sink, upb_bytesink_vtbl *vtbl) {
   sink->vtbl = vtbl;
   upb_status_init(&sink->status);

upb/bytestream.h

@@ -160,6 +160,14 @@ INLINE void upb_strref_read(const struct _upb_strref *r, char *buf) {
   }
 }
 
+// Dynamically allocates a upb_strref object whose contents are the given
+// string.  The given string data is copied into the strref, which makes these
+// functions unsuitable for tight loops (in those cases a strref should be made
+// to point to existing string data).
+upb_strref *upb_strref_new(const char *str);
+upb_strref *upb_strref_newl(const void *str, size_t len);
+void upb_strref_free(upb_strref *ref);
+
 
 /* upb_bytesink ***************************************************************/
 

upb/def.c

@@ -8,6 +8,7 @@
 #include <stdlib.h>
 #include <stddef.h>
 #include <string.h>
+#include "upb/bytestream.h"
 #include "upb/def.h"
 
 #define alignof(t) offsetof(struct { char c; t x; }, x)
@@ -194,42 +195,77 @@ const char *upb_enumdef_iton(upb_enumdef *def, int32_t num) {
   return e ? e->str : NULL;
 }
 
-bool upb_enumdef_ntoil(upb_enumdef *def, char *name, size_t len, int32_t *num) {
+bool upb_enumdef_ntoil(upb_enumdef *def, const char *name, size_t len, int32_t *num) {
   upb_ntoi_ent *e = upb_strtable_lookupl(&def->ntoi, name, len);
   if (!e) return false;
   if (num) *num = e->value;
   return true;
 }
 
-bool upb_enumdef_ntoi(upb_enumdef *e, char *name, int32_t *num) {
+bool upb_enumdef_ntoi(upb_enumdef *e, const char *name, int32_t *num) {
   return upb_enumdef_ntoil(e, name, strlen(name), num);
 }
 
 
 /* upb_fielddef ***************************************************************/
 
+static void upb_fielddef_init_default(upb_fielddef *f);
+
 upb_fielddef *upb_fielddef_new() {
   upb_fielddef *f = malloc(sizeof(*f));
   f->msgdef = NULL;
   f->def = NULL;
   upb_atomic_init(&f->refcount, 1);
   f->finalized = false;
-  f->type = 0;
   f->label = UPB_LABEL(OPTIONAL);
   f->hasbit = -1;
   f->offset = 0;
-  f->number = 0;  // not a valid field number.
-  f->hasdefault = false;
-  f->name = NULL;
   f->accessor = NULL;
   upb_value_setfielddef(&f->fval, f);
+
+  // These are initialized to be invalid; the user must set them explicitly.
+  // Could relax this later if it's convenient and non-confusing to have a
+  // defaults for them.
+  f->name = NULL;
+  f->type = 0;
+  f->number = 0;
+
+  upb_fielddef_init_default(f);
   return f;
 }
 
-static void upb_fielddef_free(upb_fielddef *f) {
-  if (upb_isstring(f)) {
-    free(upb_value_getptr(f->defaultval));
+static void upb_fielddef_init_default(upb_fielddef *f) {
+  switch (upb_fielddef_type(f)) {
+    case UPB_TYPE(DOUBLE): upb_value_setdouble(&f->defaultval, 0); break;
+    case UPB_TYPE(FLOAT): upb_value_setfloat(&f->defaultval, 0); break;
+    case UPB_TYPE(UINT64):
+    case UPB_TYPE(FIXED64): upb_value_setuint64(&f->defaultval, 0); break;
+    case UPB_TYPE(INT64):
+    case UPB_TYPE(SFIXED64):
+    case UPB_TYPE(SINT64): upb_value_setint64(&f->defaultval, 0); break;
+    case UPB_TYPE(ENUM):
+    case UPB_TYPE(INT32):
+    case UPB_TYPE(SINT32):
+    case UPB_TYPE(SFIXED32): upb_value_setint32(&f->defaultval, 0); break;
+    case UPB_TYPE(UINT32):
+    case UPB_TYPE(FIXED32): upb_value_setuint32(&f->defaultval, 0); break;
+    case UPB_TYPE(BOOL): upb_value_setbool(&f->defaultval, false); break;
+    case UPB_TYPE(STRING):
+    case UPB_TYPE(BYTES): upb_value_setstrref(&f->defaultval, upb_strref_new("")); break;
+    case UPB_TYPE(GROUP):
+    case UPB_TYPE(MESSAGE): upb_value_setptr(&f->defaultval, NULL); break;
+  }
+  f->default_is_symbolic = false;
+}
+
+static void upb_fielddef_uninit_default(upb_fielddef *f) {
+  if (upb_isstring(f) || f->default_is_symbolic) {
+    upb_strref_free((upb_strref*)upb_value_getstrref(f->defaultval));
   }
+}
+
+static void upb_fielddef_free(upb_fielddef *f) {
+  upb_fielddef_uninit_default(f);
   if (f->def) {
     // We own a ref on the subdef iff we are not part of a msgdef.
     if (f->msgdef == NULL) {
@@ -286,17 +322,18 @@ static bool upb_fielddef_resolve(upb_fielddef *f, upb_def *def, upb_status *s) {
   assert(upb_dyncast_unresolveddef(f->def));
   upb_def_unref(f->def);
   f->def = def;
-  if (f->type == UPB_TYPE(ENUM)) {
+  if (f->type == UPB_TYPE(ENUM) && f->default_is_symbolic) {
     // Resolve the enum's default from a string to an integer.
-    char *str = upb_value_getptr(f->defaultval);
+    upb_strref *str = (upb_strref*)upb_value_getstrref(f->defaultval);
     assert(str);  // Should point to either a real default or the empty string.
     upb_enumdef *e = upb_downcast_enumdef(f->def);
     int32_t val = 0;
-    if (str[0] == '\0') {
+    // Could do a sanity check that the default value does not have embedded
+    // NULLs.
+    if (str->ptr[0] == '\0') {
       upb_value_setint32(&f->defaultval, e->defaultval);
     } else {
-      bool success = upb_enumdef_ntoi(e, str, &val);
-      free(str);
+      bool success = upb_enumdef_ntoi(e, str->ptr, &val);
       if (!success) {
         upb_status_seterrf(
             s, "Default enum value (%s) is not a member of the enum", str);
@@ -304,6 +341,7 @@ static bool upb_fielddef_resolve(upb_fielddef *f, upb_def *def, upb_status *s) {
       }
       upb_value_setint32(&f->defaultval, val);
     }
+    upb_strref_free(str);
   }
   return true;
 }
@@ -323,7 +361,9 @@ bool upb_fielddef_setname(upb_fielddef *f, const char *name) {
 
 bool upb_fielddef_settype(upb_fielddef *f, uint8_t type) {
   assert(!f->finalized);
+  upb_fielddef_uninit_default(f);
   f->type = type;
+  upb_fielddef_init_default(f);
   return true;
 }
 
@@ -335,10 +375,23 @@ bool upb_fielddef_setlabel(upb_fielddef *f, uint8_t label) {
 
 void upb_fielddef_setdefault(upb_fielddef *f, upb_value value) {
   assert(!f->finalized);
-  // TODO: string ownership?
+  assert(!upb_isstring(f));
   f->defaultval = value;
 }
 
+void upb_fielddef_setdefaultstr(upb_fielddef *f, const void *str, size_t len) {
+  assert(upb_isstring(f) || f->type == UPB_TYPE(ENUM));
+  const upb_strref *ref = upb_value_getstrref(f->defaultval);
+  assert(ref);
+  upb_strref_free((upb_strref*)ref);
+  upb_value_setstrref(&f->defaultval, upb_strref_newl(str, len));
+  f->default_is_symbolic = true;
+}
+
+void upb_fielddef_setdefaultcstr(upb_fielddef *f, const char *str) {
+  upb_fielddef_setdefaultstr(f, str, str ? strlen(str) : 0);
+}
+
 void upb_fielddef_setfval(upb_fielddef *f, upb_value fval) {
   assert(!f->finalized);
   // TODO: string ownership?
@@ -728,30 +781,6 @@ bool upb_symtab_add(upb_symtab *s, upb_def **defs, int n, upb_status *status) {
         return false;
       }
 
-      // Set default default if none was set explicitly.
-      if (!f->hasdefault) {
-        switch (upb_fielddef_type(f)) {
-          case UPB_TYPE(DOUBLE): upb_value_setdouble(&f->defaultval, 0); break;
-          case UPB_TYPE(FLOAT): upb_value_setfloat(&f->defaultval, 0); break;
-          case UPB_TYPE(UINT64):
-          case UPB_TYPE(FIXED64): upb_value_setuint64(&f->defaultval, 0); break;
-          case UPB_TYPE(INT64):
-          case UPB_TYPE(SFIXED64):
-          case UPB_TYPE(SINT64): upb_value_setint64(&f->defaultval, 0); break;
-          case UPB_TYPE(INT32):
-          case UPB_TYPE(SINT32):
-          case UPB_TYPE(SFIXED32): upb_value_setint32(&f->defaultval, 0); break;
-          case UPB_TYPE(UINT32):
-          case UPB_TYPE(FIXED32): upb_value_setuint32(&f->defaultval, 0); break;
-          case UPB_TYPE(BOOL): upb_value_setbool(&f->defaultval, false); break;
-          case UPB_TYPE(ENUM):  // Will be resolved by upb_resolve().
-          case UPB_TYPE(STRING):
-          case UPB_TYPE(BYTES):
-          case UPB_TYPE(GROUP):
-          case UPB_TYPE(MESSAGE): break;  // do nothing for now.
-        }
-      }
-
       if (!upb_hassubdef(f)) continue;  // No resolving necessary.
       upb_downcast_unresolveddef(f->def);  // Type check.
       const char *name = f->def->fqname;

upb/def.h

@@ -109,7 +109,7 @@ typedef struct _upb_fielddef {
   uint8_t label;         // Use UPB_LABEL() constants.
   int16_t hasbit;
   uint16_t offset;
-  bool hasdefault;
+  bool default_is_symbolic;
   bool active;
   int32_t number;
   char *name;
@@ -133,7 +133,6 @@ INLINE uint8_t upb_fielddef_type(const upb_fielddef *f) { return f->type; }
 INLINE uint8_t upb_fielddef_label(const upb_fielddef *f) { return f->label; }
 INLINE int32_t upb_fielddef_number(const upb_fielddef *f) { return f->number; }
 INLINE char *upb_fielddef_name(const upb_fielddef *f) { return f->name; }
-INLINE upb_value upb_fielddef_default(const upb_fielddef *f) { return f->defaultval; }
 INLINE upb_value upb_fielddef_fval(const upb_fielddef *f) { return f->fval; }
 INLINE bool upb_fielddef_finalized(const upb_fielddef *f) { return f->finalized; }
 INLINE struct _upb_msgdef *upb_fielddef_msgdef(const upb_fielddef *f) {
@@ -146,6 +145,25 @@ INLINE const char *upb_fielddef_typename(const upb_fielddef *f) {
   return f->def ? f->def->fqname : NULL;
 }
 
+// Returns the default value for this fielddef, which may either be something
+// the client set explicitly or the "default default" (0 for numbers, empty for
+// strings).  The field's type indicates the type of the returned value, except
+// for enums.   For enums the default can be set either numerically or
+// symbolically -- the upb_fielddef_default_is_symbolic() function below will
+// indicate which it is.  For string defaults, the value will be a upb_strref
+// which is invalidated by any other call on this object.
+INLINE upb_value upb_fielddef_default(const upb_fielddef *f) {
+  return f->defaultval;
+}
+
+// The results of this function are only meaningful for enum fields, which can
+// have a default specified either as an integer or as a string.  If this
+// returns true, the default returned from upb_fielddef_default() is a string,
+// otherwise it is an integer.
+INLINE bool upb_fielddef_default_is_symbolic(const upb_fielddef *f) {
+  return f->default_is_symbolic;
+}
+
 // The enum or submessage def for this field, if any.  Only meaningful for
 // submessage, group, and enum fields (ie. when upb_hassubdef(f) is true).
 // Since defs are not linked together until they are in a symtab, this
@@ -161,13 +179,29 @@ bool upb_fielddef_setname(upb_fielddef *f, const char *name);
 // These writers may be called at any time prior to being put in a symtab.
 bool upb_fielddef_settype(upb_fielddef *f, uint8_t type);
 bool upb_fielddef_setlabel(upb_fielddef *f, uint8_t label);
-void upb_fielddef_setdefault(upb_fielddef *f, upb_value value);
 void upb_fielddef_setfval(upb_fielddef *f, upb_value fval);
 void upb_fielddef_setaccessor(upb_fielddef *f, struct _upb_accessor_vtbl *vtbl);
+
 // The name of the message or enum this field is referring to.  Must be found
 // at name resolution time (when upb_symtab_add() is called).
+//
+// NOTE: May only be called for fields whose type has already been set to
+// be a submessage, group, or enum!  Also, will be reset to empty if the
+// field's type is set again.
 bool upb_fielddef_settypename(upb_fielddef *f, const char *name);
 
+// The default value for the field.  For numeric types, use
+// upb_fielddef_setdefault(), and "value" must match the type of the field.
+// For string/bytes types, use upb_fielddef_setdefaultstr().
+// Enum types may use either, since the default may be set either numerically
+// or symbolically.
+//
+// NOTE: May only be called for fields whose type has already been set.
+// Also, will be reset to default if the field's type is set again.
+void upb_fielddef_setdefault(upb_fielddef *f, upb_value value);
+void upb_fielddef_setdefaultstr(upb_fielddef *f, const void *str, size_t len);
+void upb_fielddef_setdefaultcstr(upb_fielddef *f, const char *str);
+
 // A variety of tests about the type of a field.
 INLINE bool upb_issubmsgtype(upb_fieldtype_t type) {
   return type == UPB_TYPE(GROUP) || type == UPB_TYPE(MESSAGE);
@@ -330,8 +364,8 @@ void upb_enumdef_setdefault(upb_enumdef *e, int32_t val);
 bool upb_enumdef_addval(upb_enumdef *e, char *name, int32_t num);
 
 // Lookups from name to integer and vice-versa.
-bool upb_enumdef_ntoil(upb_enumdef *e, char *name, size_t len, int32_t *num);
-bool upb_enumdef_ntoi(upb_enumdef *e, char *name, int32_t *num);
+bool upb_enumdef_ntoil(upb_enumdef *e, const char *name, size_t len, int32_t *num);
+bool upb_enumdef_ntoi(upb_enumdef *e, const char *name, int32_t *num);
 // Caller does not own the returned string.
 const char *upb_enumdef_iton(upb_enumdef *e, int32_t num);
 

upb/descriptor.c

@@ -284,6 +284,8 @@ static upb_mhandlers *upb_enumdef_register_EnumDescriptorProto(upb_handlers *h)
 static upb_flow_t upb_fielddef_startmsg(void *_r) {
   upb_descreader *r = _r;
   r->f = upb_fielddef_new();
+  free(r->default_string);
+  r->default_string = NULL;
   return UPB_CONTINUE;
 }
 
@@ -291,28 +293,24 @@ static upb_flow_t upb_fielddef_startmsg(void *_r) {
 // Returns true on success.
 static bool upb_fielddef_parsedefault(char *str, upb_value *d, int type) {
   bool success = true;
-  if (type == UPB_TYPE(STRING) || type == UPB_TYPE(BYTES) || type == UPB_TYPE(ENUM)) {
-    // We'll keep the ref we had on it.  We include enums in this case because
-    // we need the enumdef to resolve the name, but we may not have it yet.
-    // We'll resolve it later.
-    if (!str) str = strdup("");
-    upb_value_setptr(d, str);
-  } else if (type == UPB_TYPE(MESSAGE) || type == UPB_TYPE(GROUP)) {
-    // We don't expect to get a default value.
-    free(str);
-    upb_value_setptr(d, NULL);
-    if (str != NULL) success = false;
-  } else if (type == UPB_TYPE(BOOL)) {
-    if (!str || strcmp(str, "false") == 0)
-      upb_value_setbool(d, false);
-    else if (strcmp(str, "true") == 0)
-      upb_value_setbool(d, true);
-    else
-      success = false;
-    free(str);
+  if (str) {
+    switch(type) {
+      case UPB_TYPE(INT32):
+      case UPB_TYPE(SINT32):
+      case UPB_TYPE(SFIXED32): upb_value_setint32(d, 0); break;
+      case UPB_TYPE(INT64):
+      case UPB_TYPE(SINT64):
+      case UPB_TYPE(SFIXED64): upb_value_setint64(d, 0); break;
+      case UPB_TYPE(UINT32):
+      case UPB_TYPE(FIXED32): upb_value_setuint32(d, 0);
+      case UPB_TYPE(UINT64):
+      case UPB_TYPE(FIXED64): upb_value_setuint64(d, 0); break;
+      case UPB_TYPE(DOUBLE): upb_value_setdouble(d, 0); break;
+      case UPB_TYPE(FLOAT): upb_value_setfloat(d, 0); break;
+      case UPB_TYPE(BOOL): upb_value_setbool(d, false); break;
+      default: abort();
+    }
   } else {
-    // The strto* functions need the string to be NULL-terminated.
-    if (!str) str = strdup("0");
     char *end;
     switch (type) {
       case UPB_TYPE(INT32):
@@ -353,9 +351,16 @@ static bool upb_fielddef_parsedefault(char *str, upb_value *d, int type) {
         upb_value_setfloat(d, strtof(str, &end));
         if (errno == ERANGE || *end) success = false;
         break;
+      case UPB_TYPE(BOOL): {
+        if (strcmp(str, "false") == 0)
+          upb_value_setbool(d, false);
+        else if (strcmp(str, "true") == 0)
+          upb_value_setbool(d, true);
+        else
+          success = false;
+      }
       default: abort();
     }
-    free(str);
   }
   return success;
 }
@@ -372,17 +377,26 @@ static void upb_fielddef_endmsg(void *_r, upb_status *status) {
   upb_msgdef_addfield(m, f);
   upb_fielddef_unref(f);
   r->f = NULL;
-  char *dstr = r->default_string;
-  r->default_string = NULL;
-  upb_value val;
-  upb_value_setptr(&val, NULL);  // Silence inaccurate compiler warnings.
-  if (!upb_fielddef_parsedefault(dstr, &val, f->type)) {
-    // We don't worry too much about giving a great error message since the
-    // compiler should have ensured this was correct.
-    upb_status_seterrliteral(status, "Error converting default value.");
-    return;
+
+  if (r->default_string) {
+    if (upb_issubmsg(f)) {
+      upb_status_seterrliteral(status, "Submessages cannot have defaults.");
+      return;
+    }
+    if (upb_isstring(f) || f->type == UPB_TYPE(ENUM)) {
+      upb_fielddef_setdefaultcstr(f, r->default_string);
+    } else {
+      upb_value val;
+      upb_value_setptr(&val, NULL);  // Silence inaccurate compiler warnings.
+      if (!upb_fielddef_parsedefault(r->default_string, &val, f->type)) {
+        // We don't worry too much about giving a great error message since the
+        // compiler should have ensured this was correct.
+        upb_status_seterrliteral(status, "Error converting default value.");
+        return;
+      }
+      upb_fielddef_setdefault(f, val);
+    }
   }
-  upb_fielddef_setdefault(f, val);
 }
 
 static upb_flow_t upb_fielddef_ontype(void *_r, upb_value fval, upb_value val) {

upb/upb.h

@@ -162,6 +162,15 @@ typedef struct {
 #define SET_TYPE(dest, val) dest = val
 #endif
 
+// For each value type, define the following set of functions:
+//
+// // Get/set an int32 from a upb_value.
+// int32_t upb_value_getint32(upb_value val);
+// void upb_value_setint32(upb_value *val, int32_t cval);
+//
+// // Construct a new upb_value from an int32.
+// upb_value upb_value_int32(int32_t val);
+
 #define UPB_VALUE_ACCESSORS(name, membername, ctype, proto_type) \
   INLINE ctype upb_value_get ## name(upb_value val) { \
     assert(val.type == proto_type); \
@@ -170,7 +179,13 @@ typedef struct {
   INLINE void upb_value_set ## name(upb_value *val, ctype cval) { \
     SET_TYPE(val->type, proto_type); \
     val->val.membername = cval; \
+  } \
+  INLINE upb_value upb_value_ ## name(ctype val) { \
+    upb_value ret; \
+    upb_value_set ## name(&ret, val); \
+    return ret; \
   }
+
 UPB_VALUE_ACCESSORS(double, _double, double, UPB_TYPE(DOUBLE));
 UPB_VALUE_ACCESSORS(float, _float, float, UPB_TYPE(FLOAT));
 UPB_VALUE_ACCESSORS(int32, int32, int32_t, UPB_TYPE(INT32));