Merge branch 'master' into google-internal

8 files changed, 173 insertions, 147 deletions

diff --git a/tests/bindings/googlepb/test_vs_proto2.cc b/tests/bindings/googlepb/test_vs_proto2.cc
index 8e68791..06bea87 100644
--- a/tests/bindings/googlepb/test_vs_proto2.cc
+++ b/tests/bindings/googlepb/test_vs_proto2.cc
@@ -47,7 +47,7 @@ void compare_metadata(const google::protobuf::Descriptor* d,
         d->FindFieldByNumber(upb_f->number());
     ASSERT(upb_f);
     ASSERT(proto2_f);
-    ASSERT(upb_f->number() == proto2_f->number());
+    ASSERT(upb_f->number() == (uint32_t)proto2_f->number());
     ASSERT(std::string(upb_f->name()) == proto2_f->name());
     ASSERT(upb_f->descriptor_type() ==
            static_cast<upb::FieldDef::DescriptorType>(proto2_f->type()));
@@ -124,6 +124,8 @@ extern "C" {
 int run_tests(int argc, char *argv[]) {
   UPB_UNUSED(argc);
   UPB_UNUSED(argv);
+  UPB_UNUSED(message1_data);
+  UPB_UNUSED(message2_data);
   size_t len = sizeof(MESSAGE_DATA_IDENT);
   const char *str = (const char*)MESSAGE_DATA_IDENT;
 
diff --git a/tests/bindings/lua/test_upb.pb.lua b/tests/bindings/lua/test_upb.pb.lua
index 02b443a..7c1c0d0 100644
--- a/tests/bindings/lua/test_upb.pb.lua
+++ b/tests/bindings/lua/test_upb.pb.lua
@@ -1,6 +1,8 @@
 
-local upb = require "upb"
+-- Require "pb" first to ensure that the transitive require of "upb" is
+-- handled properly by the "pb" module.
 local pb = require "upb.pb"
+local upb = require "upb"
 local lunit = require "lunit"
 
 if _VERSION >= 'Lua 5.2' then
diff --git a/tests/pb/test_decoder.cc b/tests/pb/test_decoder.cc
index 98926a6..c615a3c 100644
--- a/tests/pb/test_decoder.cc
+++ b/tests/pb/test_decoder.cc
@@ -111,7 +111,7 @@ using std::string;
 
 void vappendf(string* str, const char *format, va_list args) {
   va_list copy;
-  va_copy(copy, args);
+  __va_copy(copy, args);
 
   int count = vsnprintf(NULL, 0, format, args);
   if (count >= 0)
@@ -577,7 +577,7 @@ void run_decoder(const string& proto, const string* expected_output) {
           fprintf(stderr, "RUNNING TEST CASE, hash=%x\n", testhash);
           fprintf(stderr, "JIT on: %s\n",
                   global_method->is_native() ? "true" : "false");
-          fprintf(stderr, "Input (len=%zu): ", proto.size());
+          fprintf(stderr, "Input (len=%u): ", (unsigned)proto.size());
           PrintBinary(proto);
           fprintf(stderr, "\n");
           if (expected_output) {
diff --git a/tests/pb/test_varint.c b/tests/pb/test_varint.c
index 84ff831..c80f127 100644
--- a/tests/pb/test_varint.c
+++ b/tests/pb/test_varint.c
@@ -8,25 +8,30 @@
 #include "upb/pb/varint.int.h"
 #include "tests/upb_test.h"
 
-// Test that we can round-trip from int->varint->int.
+/* Test that we can round-trip from int->varint->int. */
 static void test_varint_for_num(upb_decoderet (*decoder)(const char*),
                                 uint64_t num) {
   char buf[16];
+  size_t bytes;
+  upb_decoderet r;
+
   memset(buf, 0xff, sizeof(buf));
-  size_t bytes = upb_vencode64(num, buf);
+  bytes = upb_vencode64(num, buf);
 
   if (num <= UINT32_MAX) {
+    uint64_t encoded = upb_vencode32(num);
     char buf2[16];
+    upb_decoderet r;
+
     memset(buf2, 0, sizeof(buf2));
-    uint64_t encoded = upb_vencode32(num);
     memcpy(&buf2, &encoded, 8);
-    upb_decoderet r = decoder(buf2);
+    r = decoder(buf2);
     ASSERT(r.val == num);
     ASSERT(r.p == buf2 + upb_value_size(encoded));
     ASSERT(upb_zzenc_32(upb_zzdec_32(num)) == num);
   }
 
-  upb_decoderet r = decoder(buf);
+  r = decoder(buf);
   ASSERT(r.val == num);
   ASSERT(r.p == buf + bytes);
   ASSERT(upb_zzenc_64(upb_zzdec_64(num)) == num);
@@ -36,13 +41,23 @@ static void test_varint_decoder(upb_decoderet (*decoder)(const char*)) {
 #define TEST(bytes, expected_val) {\
     size_t n = sizeof(bytes) - 1;  /* for NULL */ \
     char buf[UPB_PB_VARINT_MAX_LEN]; \
+    upb_decoderet r; \
     memset(buf, 0xff, sizeof(buf)); \
     memcpy(buf, bytes, n); \
-    upb_decoderet r = decoder(buf); \
+    r = decoder(buf); \
     ASSERT(r.val == expected_val); \
     ASSERT(r.p == buf + n); \
   }
 
+  uint64_t num;
+
+  char twelvebyte[16] = {-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 1, 1};
+  const char *twelvebyte_buf = twelvebyte;
+  /* A varint that terminates before hitting the end of the provided buffer,
+   * but in too many bytes (11 instead of 10). */
+  upb_decoderet r = decoder(twelvebyte_buf);
+  ASSERT(r.p == NULL);
+
   TEST("\x00",                                                      0ULL);
   TEST("\x01",                                                      1ULL);
   TEST("\x81\x14",                                              0xa01ULL);
@@ -57,16 +72,7 @@ static void test_varint_decoder(upb_decoderet (*decoder)(const char*)) {
   TEST("\x81\x83\x87\x8f\x9f\xbf\xff\x81\x83\x07", 0x8303fdf9f1e1c181ULL);
 #undef TEST
 
-  char twelvebyte[16] = {0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
-                         0x80, 0x01, 0x01};
-  const char *twelvebyte_buf = twelvebyte;
-  // A varint that terminates before hitting the end of the provided buffer,
-  // but in too many bytes (11 instead of 10).
-  upb_decoderet r = decoder(twelvebyte_buf);
-  ASSERT(r.p == NULL);
-
-
-  for (uint64_t num = 5; num * 1.5 < UINT64_MAX; num *= 1.5) {
+  for (num = 5; num * 1.5 < UINT64_MAX; num *= 1.5) {
     test_varint_for_num(decoder, num);
   }
   test_varint_for_num(decoder, 0);
@@ -80,16 +86,13 @@ static void test_varint_decoder(upb_decoderet (*decoder)(const char*)) {
     return upb_vdecode_ ## decoder(p); \
   } \
   void test_ ## decoder() { \
-    printf("Testing varint decoder: " #decoder "..."); \
-    fflush(stdout); \
     test_varint_decoder(&_upb_vdecode_ ## decoder); \
-    printf("ok.\n"); \
   } \
 
-TEST_VARINT_DECODER(check2_branch32);
-TEST_VARINT_DECODER(check2_branch64);
-TEST_VARINT_DECODER(check2_wright);
-TEST_VARINT_DECODER(check2_massimino);
+TEST_VARINT_DECODER(check2_branch32)
+TEST_VARINT_DECODER(check2_branch64)
+TEST_VARINT_DECODER(check2_wright)
+TEST_VARINT_DECODER(check2_massimino)
 
 int run_tests(int argc, char *argv[]) {
   UPB_UNUSED(argc);
diff --git a/tests/test_def.c b/tests/test_def.c
index 800d685..4f9fcff 100644
--- a/tests/test_def.c
+++ b/tests/test_def.c
@@ -20,23 +20,23 @@ static void test_empty_symtab() {
   upb_symtab_iter i;
   for (upb_symtab_begin(&i, s, UPB_DEF_ANY); !upb_symtab_done(&i);
        upb_symtab_next(&i)) {
-    ASSERT(false);  // Should not get here.
+    ASSERT(false);  /* Should not get here. */
   }
   upb_symtab_unref(s, &s);
 }
 
 static void test_noreftracking() {
-  // Reftracking is not required; clients can pass UPB_UNTRACKED_REF for owner.
+  /* Reftracking is not required; clients can pass UPB_UNTRACKED_REF for owner. */
   upb_msgdef *md = upb_msgdef_new(UPB_UNTRACKED_REF);
   upb_msgdef_ref(md, UPB_UNTRACKED_REF);
 
-  // Clients can mix tracked and untracked refs.
+  /* Clients can mix tracked and untracked refs. */
   upb_msgdef_ref(md, &md);
 
   upb_msgdef_unref(md, UPB_UNTRACKED_REF);
   upb_msgdef_unref(md, UPB_UNTRACKED_REF);
 
-  // Call some random function on the messagedef to test that it is alive.
+  /* Call some random function on the messagedef to test that it is alive. */
   ASSERT(!upb_msgdef_isfrozen(md));
 
   upb_msgdef_unref(md, &md);
@@ -44,8 +44,8 @@ static void test_noreftracking() {
 
 static upb_symtab *load_test_proto(void *owner) {
   upb_symtab *s = upb_symtab_new(owner);
-  ASSERT(s);
   upb_status status = UPB_STATUS_INIT;
+  ASSERT(s);
   if (!upb_load_descriptor_file_into_symtab(s, descriptor_file, &status)) {
     fprintf(stderr, "Error loading descriptor file: %s\n",
             upb_status_errmsg(&status));
@@ -58,60 +58,73 @@ static upb_symtab *load_test_proto(void *owner) {
 }
 
 static void test_cycles() {
+  bool ok;
   upb_symtab *s = load_test_proto(&s);
-
-  // Test cycle detection by making a cyclic def's main refcount go to zero
-  // and then be incremented to one again.
-  const upb_def *def = upb_symtab_lookup(s, "A");
+  const upb_msgdef *m;
+  const upb_fielddef *f;
+  const upb_def *def;
+  const upb_def *def2;
+
+  /* Test cycle detection by making a cyclic def's main refcount go to zero
+   * and then be incremented to one again. */
+  def = upb_symtab_lookup(s, "A");
   upb_def_ref(def, &def);
   ASSERT(def);
   ASSERT(upb_def_isfrozen(def));
   upb_symtab_unref(s, &s);
 
-  // Message A has only one subfield: "optional B b = 1".
-  const upb_msgdef *m = upb_downcast_msgdef(def);
-  const upb_fielddef *f = upb_msgdef_itof(m, 1);
+  /* Message A has only one subfield: "optional B b = 1". */
+  m = upb_downcast_msgdef(def);
+  f = upb_msgdef_itof(m, 1);
   ASSERT(f);
   ASSERT(upb_fielddef_hassubdef(f));
   ASSERT(upb_msgdef_ntofz(m, "b") == f);
   ASSERT(upb_msgdef_ntof(m, "b", 1) == f);
-  const upb_def *def2 = upb_fielddef_subdef(f);
+  def2 = upb_fielddef_subdef(f);
   ASSERT(upb_downcast_msgdef(def2));
-  ASSERT(strcmp(upb_def_fullname(def2), "B") == 0);
+  ok = strcmp(upb_def_fullname(def2), "B") == 0;
+  ASSERT(ok);
 
   upb_def_ref(def2, &def2);
   upb_def_unref(def, &def);
 
-  // We know "def" is still alive because it's reachable from def2.
-  ASSERT(strcmp(upb_def_fullname(def), "A") == 0);
+  /* We know "def" is still alive because it's reachable from def2. */
+  ok = strcmp(upb_def_fullname(def), "A") == 0;
+  ASSERT(ok);
   upb_def_unref(def2, &def2);
 }
 
 static void test_symbol_resolution() {
   upb_status s = UPB_STATUS_INIT;
+  upb_def *defs[2];
+  upb_msgdef *m1;
+  upb_msgdef *m2;
+  upb_msgdef *m3;
+  upb_fielddef *m3_field1;
+  upb_fielddef *m3_field2;
 
   upb_symtab *symtab = upb_symtab_new(&symtab);
   ASSERT(symtab);
 
-  // m1 has name "A.B.C" and no fields. We'll add it to the symtab now.
-  upb_msgdef *m1 = upb_msgdef_new(&m1);
+  /* m1 has name "A.B.C" and no fields. We'll add it to the symtab now. */
+  m1 = upb_msgdef_new(&m1);
   ASSERT(m1);
   ASSERT_STATUS(upb_msgdef_setfullname(m1, "A.B.C", &s), &s);
   ASSERT_STATUS(upb_symtab_add(symtab, (upb_def**)&m1, 1,
                                NULL, &s), &s);
 
-  // m2 has name "D.E" and no fields. We'll add it in the same batch as m3
-  // below.
-  upb_msgdef *m2 = upb_msgdef_new(&m2);
+  /* m2 has name "D.E" and no fields. We'll add it in the same batch as m3
+   * below. */
+  m2 = upb_msgdef_new(&m2);
   ASSERT(m2);
   ASSERT_STATUS(upb_msgdef_setfullname(m2, "D.E", &s), &s);
 
-  // m3 has name "F.G" and two fields, of type A.B.C and D.E respectively. We'll
-  // add it in the same batch as m2 above.
-  upb_msgdef *m3 = upb_msgdef_new(&m3);
+  /* m3 has name "F.G" and two fields, of type A.B.C and D.E respectively. We'll
+   * add it in the same batch as m2 above. */
+  m3 = upb_msgdef_new(&m3);
   ASSERT(m3);
   ASSERT_STATUS(upb_msgdef_setfullname(m3, "F.G", &s), &s);
-  upb_fielddef *m3_field1 = upb_fielddef_new(&m3_field1);
+  m3_field1 = upb_fielddef_new(&m3_field1);
   ASSERT_STATUS(upb_fielddef_setname(m3_field1, "field1", &s), &s);
   ASSERT_STATUS(upb_fielddef_setnumber(m3_field1, 1, &s), &s);
   upb_fielddef_setlabel(m3_field1, UPB_LABEL_OPTIONAL);
@@ -119,7 +132,7 @@ static void test_symbol_resolution() {
   ASSERT_STATUS(upb_fielddef_setsubdefname(m3_field1, ".A.B.C", &s), &s);
   ASSERT_STATUS(upb_msgdef_addfield(m3, m3_field1, NULL, &s), &s);
 
-  upb_fielddef *m3_field2 = upb_fielddef_new(&m3_field2);
+  m3_field2 = upb_fielddef_new(&m3_field2);
   ASSERT_STATUS(upb_fielddef_setname(m3_field2, "field2", &s), &s);
   ASSERT_STATUS(upb_fielddef_setnumber(m3_field2, 2, &s), &s);
   upb_fielddef_setlabel(m3_field2, UPB_LABEL_OPTIONAL);
@@ -127,7 +140,8 @@ static void test_symbol_resolution() {
   ASSERT_STATUS(upb_fielddef_setsubdefname(m3_field2, ".D.E", &s), &s);
   ASSERT_STATUS(upb_msgdef_addfield(m3, m3_field2, NULL, &s), &s);
 
-  upb_def *defs[2] = { (upb_def*)m2, (upb_def*)m3 };
+  defs[0] = upb_msgdef_upcast_mutable(m2);
+  defs[1] = upb_msgdef_upcast_mutable(m3);
   ASSERT_STATUS(upb_symtab_add(symtab, defs, 2, NULL, &s), &s);
 
   upb_fielddef_unref(m3_field2, &m3_field2);
@@ -139,43 +153,25 @@ static void test_symbol_resolution() {
 }
 
 static void test_fielddef_unref() {
+  bool ok;
   upb_symtab *s = load_test_proto(&s);
   const upb_msgdef *md = upb_symtab_lookupmsg(s, "A");
   const upb_fielddef *f = upb_msgdef_itof(md, 1);
   upb_fielddef_ref(f, &f);
 
-  // Unref symtab; now fielddef is the only thing keeping the msgdef alive.
+  /* Unref symtab; now fielddef is the only thing keeping the msgdef alive. */
   upb_symtab_unref(s, &s);
-  // Check that md is still alive.
-  ASSERT(strcmp(upb_msgdef_fullname(md), "A") == 0);
+  /* Check that md is still alive. */
+  ok = strcmp(upb_msgdef_fullname(md), "A") == 0;
+  ASSERT(ok);
 
-  // Check that unref of fielddef frees the whole remaining graph.
+  /* Check that unref of fielddef frees the whole remaining graph. */
   upb_fielddef_unref(f, &f);
 }
 
-static void test_fielddef_accessors() {
+static void test_fielddef() {
+  /* Test that we don't leak an unresolved subdef name. */
   upb_fielddef *f1 = upb_fielddef_new(&f1);
-  upb_fielddef *f2 = upb_fielddef_new(&f2);
-
-  ASSERT(!upb_fielddef_isfrozen(f1));
-  ASSERT(upb_fielddef_setname(f1, "f1", NULL));
-  ASSERT(upb_fielddef_setnumber(f1, 1937, NULL));
-  upb_fielddef_settype(f1, UPB_TYPE_INT64);
-  upb_fielddef_setlabel(f1, UPB_LABEL_REPEATED);
-  ASSERT(upb_fielddef_number(f1) == 1937);
-
-  ASSERT(!upb_fielddef_isfrozen(f2));
-  ASSERT(upb_fielddef_setname(f2, "f2", NULL));
-  ASSERT(upb_fielddef_setnumber(f2, 123456789, NULL));
-  upb_fielddef_settype(f2, UPB_TYPE_BYTES);
-  upb_fielddef_setlabel(f2, UPB_LABEL_REPEATED);
-  ASSERT(upb_fielddef_number(f2) == 123456789);
-
-  upb_fielddef_unref(f1, &f1);
-  upb_fielddef_unref(f2, &f2);
-
-  // Test that we don't leak an unresolved subdef name.
-  f1 = upb_fielddef_new(&f1);
   upb_fielddef_settype(f1, UPB_TYPE_MESSAGE);
   ASSERT(upb_fielddef_setsubdefname(f1, "YO", NULL));
   upb_fielddef_unref(f1, &f1);
@@ -209,51 +205,61 @@ static upb_enumdef *upb_enumdef_newnamed(const char *name, void *owner) {
 
 static void test_replacement() {
   upb_symtab *s = upb_symtab_new(&s);
+  upb_enumdef *e2;
+  upb_msgdef *m2;
+  upb_enumdef *e;
+  upb_status status = UPB_STATUS_INIT;
+  upb_def *newdefs[3];
+  upb_def *newdefs2[1];
+  const upb_msgdef *m3;
 
   upb_msgdef *m = upb_msgdef_newnamed("MyMessage", &s);
   upb_msgdef_addfield(m, newfield("field1", 1, UPB_TYPE_ENUM,
                                   UPB_LABEL_OPTIONAL, ".MyEnum", &s),
                       &s, NULL);
-  upb_msgdef *m2 = upb_msgdef_newnamed("MyMessage2", &s);
-  upb_enumdef *e = upb_enumdef_newnamed("MyEnum", &s);
-  upb_status status = UPB_STATUS_INIT;
+  m2 = upb_msgdef_newnamed("MyMessage2", &s);
+  e = upb_enumdef_newnamed("MyEnum", &s);
   ASSERT_STATUS(upb_enumdef_addval(e, "VAL1", 1, &status), &status);
 
-  upb_def *newdefs[] = {UPB_UPCAST(m), UPB_UPCAST(m2), UPB_UPCAST(e)};
+  newdefs[0] = upb_msgdef_upcast_mutable(m);
+  newdefs[1] = upb_msgdef_upcast_mutable(m2);
+  newdefs[2] = upb_enumdef_upcast_mutable(e);
   ASSERT_STATUS(upb_symtab_add(s, newdefs, 3, &s, &status), &status);
 
-  // Try adding a new definition of MyEnum, MyMessage should get replaced with
-  // a new version.
-  upb_enumdef *e2 = upb_enumdef_newnamed("MyEnum", &s);
+  /* Try adding a new definition of MyEnum, MyMessage should get replaced with
+   * a new version. */
+  e2 = upb_enumdef_newnamed("MyEnum", &s);
   ASSERT_STATUS(upb_enumdef_addval(e2, "VAL1", 1, &status), &status);
-  upb_def *newdefs2[] = {UPB_UPCAST(e2)};
+  newdefs2[0] = upb_enumdef_upcast_mutable(e2);
   ASSERT_STATUS(upb_symtab_add(s, newdefs2, 1, &s, &status), &status);
 
-  const upb_msgdef *m3 = upb_symtab_lookupmsg(s, "MyMessage");
+  m3 = upb_symtab_lookupmsg(s, "MyMessage");
   ASSERT(m3);
-  // Must be different because it points to MyEnum which was replaced.
+  /* Must be different because it points to MyEnum which was replaced. */
   ASSERT(m3 != m);
 
   m3 = upb_symtab_lookupmsg(s, "MyMessage2");
-  // Should be the same because it was not replaced, nor were any defs that
-  // are reachable from it.
+  /* Should be the same because it was not replaced, nor were any defs that
+   * are reachable from it. */
   ASSERT(m3 == m2);
 
   upb_symtab_unref(s, &s);
 }
 
 static void test_freeze_free() {
-  // Test that freeze frees defs that were only being kept alive by virtue of
-  // sharing a group with other defs that are being frozen.
+  bool ok;
+
+  /* Test that freeze frees defs that were only being kept alive by virtue of
+   * sharing a group with other defs that are being frozen. */
   upb_msgdef *m1 = upb_msgdef_newnamed("M1", &m1);
   upb_msgdef *m2 = upb_msgdef_newnamed("M2", &m2);
   upb_msgdef *m3 = upb_msgdef_newnamed("M3", &m3);
   upb_msgdef *m4 = upb_msgdef_newnamed("M4", &m4);
+  upb_fielddef *f = upb_fielddef_new(&f);
 
-  // Freeze M4 and make M1 point to it.
+  /* Freeze M4 and make M1 point to it. */
   upb_def_freeze((upb_def*const*)&m4, 1, NULL);
 
-  upb_fielddef *f = upb_fielddef_new(&f);
   upb_fielddef_settype(f, UPB_TYPE_MESSAGE);
   ASSERT(upb_fielddef_setnumber(f, 1, NULL));
   ASSERT(upb_fielddef_setname(f, "foo", NULL));
@@ -261,10 +267,10 @@ static void test_freeze_free() {
 
   ASSERT(upb_msgdef_addfield(m1, f, &f, NULL));
 
-  // After this unref, M1 is the only thing keeping M4 alive.
+  /* After this unref, M1 is the only thing keeping M4 alive. */
   upb_msgdef_unref(m4, &m4);
 
-  // Force M1/M2/M3 into a single mutable refcounting group.
+  /* Force M1/M2/M3 into a single mutable refcounting group. */
   f = upb_fielddef_new(&f);
   upb_fielddef_settype(f, UPB_TYPE_MESSAGE);
   ASSERT(upb_fielddef_setnumber(f, 1, NULL));
@@ -274,40 +280,45 @@ static void test_freeze_free() {
   ASSERT(upb_fielddef_setmsgsubdef(f, m2, NULL));
   ASSERT(upb_fielddef_setmsgsubdef(f, m3, NULL));
 
-  // Make M3 cyclic with itself.
+  /* Make M3 cyclic with itself. */
   ASSERT(upb_msgdef_addfield(m3, f, &f, NULL));
 
-  // These will be kept alive since they are in the same refcounting group as
-  // M3, which still has a ref.  Note: this behavior is not guaranteed by the
-  // API, but true in practice with its current implementation.
+  /* These will be kept alive since they are in the same refcounting group as
+   * M3, which still has a ref.  Note: this behavior is not guaranteed by the
+   * API, but true in practice with its current implementation. */
   upb_msgdef_unref(m1, &m1);
   upb_msgdef_unref(m2, &m2);
 
-  // Test that they are still alive (NOT allowed by the API).
-  ASSERT(strcmp("M1", upb_msgdef_fullname(m1)) == 0);
-  ASSERT(strcmp("M2", upb_msgdef_fullname(m2)) == 0);
+  /* Test that they are still alive (NOT allowed by the API). */
+  ok = strcmp("M1", upb_msgdef_fullname(m1)) == 0;
+  ASSERT(ok);
+  ok = strcmp("M2", upb_msgdef_fullname(m2)) == 0;
+  ASSERT(ok);
 
-  // Freeze M3.  If the test leaked no memory, then freeing m1 and m2 was
-  // successful.
+  /* Freeze M3.  If the test leaked no memory, then freeing m1 and m2 was
+   * successful. */
   ASSERT(upb_def_freeze((upb_def*const*)&m3, 1, NULL));
 
   upb_msgdef_unref(m3, &m3);
 }
 
 static void test_partial_freeze() {
-  // Test that freeze of only part of the graph correctly adjusts objects that
-  // point to the newly-frozen objects.
+  /* Test that freeze of only part of the graph correctly adjusts objects that
+   * point to the newly-frozen objects. */
   upb_msgdef *m1 = upb_msgdef_newnamed("M1", &m1);
   upb_msgdef *m2 = upb_msgdef_newnamed("M2", &m2);
   upb_msgdef *m3 = upb_msgdef_newnamed("M3", &m3);
-
   upb_fielddef *f1 = upb_fielddef_new(&f1);
+  upb_fielddef *f2 = upb_fielddef_new(&f2);
+  upb_def *defs[2];
+  defs[0] = upb_msgdef_upcast_mutable(m1);
+  defs[1] = upb_msgdef_upcast_mutable(m2);
+
   upb_fielddef_settype(f1, UPB_TYPE_MESSAGE);
   ASSERT(upb_fielddef_setnumber(f1, 1, NULL));
   ASSERT(upb_fielddef_setname(f1, "f1", NULL));
   ASSERT(upb_fielddef_setmsgsubdef(f1, m1, NULL));
 
-  upb_fielddef *f2 = upb_fielddef_new(&f2);
   upb_fielddef_settype(f2, UPB_TYPE_MESSAGE);
   ASSERT(upb_fielddef_setnumber(f2, 2, NULL));
   ASSERT(upb_fielddef_setname(f2, "f2", NULL));
@@ -316,9 +327,8 @@ static void test_partial_freeze() {
   ASSERT(upb_msgdef_addfield(m3, f1, &f1, NULL));
   ASSERT(upb_msgdef_addfield(m3, f2, &f2, NULL));
 
-  // Freeze M1 and M2, which should cause the group to be split
-  // and m3 (left mutable) to take references on m1 and m2.
-  upb_def *defs[] = {UPB_UPCAST(m1), UPB_UPCAST(m2)};
+  /* Freeze M1 and M2, which should cause the group to be split
+   * and m3 (left mutable) to take references on m1 and m2. */
   ASSERT(upb_def_freeze(defs, 2, NULL));
 
   ASSERT(upb_msgdef_isfrozen(m1));
@@ -332,13 +342,15 @@ static void test_partial_freeze() {
 
 static void test_descriptor_flags() {
   upb_msgdef *m = upb_msgdef_new(&m);
-  ASSERT(upb_msgdef_mapentry(m) == false);
+  upb_msgdef *m2;
   upb_status s = UPB_STATUS_INIT;
+
+  ASSERT(upb_msgdef_mapentry(m) == false);
   upb_msgdef_setfullname(m, "TestMessage", &s);
   ASSERT(upb_ok(&s));
   upb_msgdef_setmapentry(m, true);
   ASSERT(upb_msgdef_mapentry(m) == true);
-  upb_msgdef *m2 = upb_msgdef_dup(m, &m2);
+  m2 = upb_msgdef_dup(m, &m2);
   ASSERT(upb_msgdef_mapentry(m2) == true);
   upb_msgdef_unref(m, &m);
   upb_msgdef_unref(m2, &m2);
@@ -346,10 +358,13 @@ static void test_descriptor_flags() {
 
 static void test_mapentry_check() {
   upb_status s = UPB_STATUS_INIT;
-
   upb_msgdef *m = upb_msgdef_new(&m);
-  upb_msgdef_setfullname(m, "TestMessage", &s);
   upb_fielddef *f = upb_fielddef_new(&f);
+  upb_symtab *symtab = upb_symtab_new(&symtab);
+  upb_msgdef *subm = upb_msgdef_new(&subm);
+  upb_def *defs[2];
+
+  upb_msgdef_setfullname(m, "TestMessage", &s);
   upb_fielddef_setname(f, "field1", &s);
   upb_fielddef_setnumber(f, 1, &s);
   upb_fielddef_setlabel(f, UPB_LABEL_OPTIONAL);
@@ -358,14 +373,13 @@ static void test_mapentry_check() {
   upb_msgdef_addfield(m, f, &f, &s);
   ASSERT(upb_ok(&s));
 
-  upb_msgdef *subm = upb_msgdef_new(&subm);
   upb_msgdef_setfullname(subm, "MapEntry", &s);
   upb_msgdef_setmapentry(subm, true);
 
-  upb_symtab *symtab = upb_symtab_new(&symtab);
-  upb_def *defs[] = {UPB_UPCAST(m), UPB_UPCAST(subm)};
+  defs[0] = upb_msgdef_upcast_mutable(m);
+  defs[1] = upb_msgdef_upcast_mutable(subm);
   upb_symtab_add(symtab, defs, 2, NULL, &s);
-  // Should not have succeeded: non-repeated field pointing to a MapEntry.
+  /* Should not have succeeded: non-repeated field pointing to a MapEntry. */
   ASSERT(!upb_ok(&s));
 
   upb_fielddef_setlabel(f, UPB_LABEL_REPEATED);
@@ -380,22 +394,26 @@ static void test_mapentry_check() {
 static void test_oneofs() {
   upb_status s = UPB_STATUS_INIT;
   bool ok = true;
-
+  upb_def *subm_defs[1];
   upb_symtab *symtab = upb_symtab_new(&symtab);
+  upb_msgdef *subm = upb_msgdef_newnamed("SubMessage", &symtab);
+  upb_msgdef *m = upb_msgdef_newnamed("TestMessage", &symtab);
+  upb_oneofdef *o = upb_oneofdef_new(&o);
+  const upb_oneofdef *lookup_o;
+  const upb_fielddef *lookup_field;
+  upb_def *defs[1];
+
   ASSERT(symtab != NULL);
 
-  // Create a test message for fields to refer to.
-  upb_msgdef *subm = upb_msgdef_newnamed("SubMessage", &symtab);
+  /* Create a test message for fields to refer to. */
   upb_msgdef_addfield(subm, newfield("field1", 1, UPB_TYPE_INT32,
                                      UPB_LABEL_OPTIONAL, NULL, &symtab),
                       &symtab, NULL);
-  upb_def *subm_defs[] = {UPB_UPCAST(subm)};
+  subm_defs[0] = upb_msgdef_upcast_mutable(subm);
   ASSERT_STATUS(upb_symtab_add(symtab, subm_defs, 1, &symtab, &s), &s);
 
-  upb_msgdef *m = upb_msgdef_newnamed("TestMessage", &symtab);
   ASSERT(upb_msgdef_numoneofs(m) == 0);
 
-  upb_oneofdef *o = upb_oneofdef_new(&o);
   ASSERT(upb_oneofdef_numfields(o) == 0);
   ASSERT(upb_oneofdef_name(o) == NULL);
 
@@ -415,14 +433,14 @@ static void test_oneofs() {
   ok = upb_msgdef_addoneof(m, o, NULL, &s);
   ASSERT_STATUS(ok, &s);
 
-  upb_def *defs[] = {UPB_UPCAST(m)};
+  defs[0] = upb_msgdef_upcast_mutable(m);
   ASSERT_STATUS(upb_symtab_add(symtab, defs, 1, &symtab, &s), &s);
 
   ASSERT(upb_msgdef_numoneofs(m) == 1);
-  const upb_oneofdef *lookup_o = upb_msgdef_ntooz(m, "test_oneof");
+  lookup_o = upb_msgdef_ntooz(m, "test_oneof");
   ASSERT(lookup_o == o);
 
-  const upb_fielddef *lookup_field = upb_oneofdef_ntofz(o, "field1");
+  lookup_field = upb_oneofdef_ntofz(o, "field1");
   ASSERT(lookup_field != NULL && upb_fielddef_number(lookup_field) == 1);
 
   upb_symtab_unref(symtab, &symtab);
@@ -438,7 +456,7 @@ int run_tests(int argc, char *argv[]) {
   test_empty_symtab();
   test_cycles();
   test_symbol_resolution();
-  test_fielddef_accessors();
+  test_fielddef();
   test_fielddef_unref();
   test_replacement();
   test_freeze_free();
diff --git a/tests/test_handlers.c b/tests/test_handlers.c
index 951f2c5..be95c0a 100644
--- a/tests/test_handlers.c
+++ b/tests/test_handlers.c
@@ -18,13 +18,13 @@ static bool startmsg(void *c, const void *hd) {
 }
 
 static void test_error() {
-  // Test creating handlers of a static msgdef.
+  /* Test creating handlers of a static msgdef. */
   const upb_symtab *s = upbdefs_google_protobuf_descriptor(&s);
   upb_handlers *h =
       upb_handlers_new(upbdefs_google_protobuf_DescriptorProto(s), &h);
   upb_symtab_unref(s, &s);
 
-  // Attempt to set the same handler twice causes error.
+  /* Attempt to set the same handler twice causes error. */
   ASSERT(upb_ok(upb_handlers_status(h)));
   upb_handlers_setstartmsg(h, &startmsg, NULL);
   ASSERT(upb_ok(upb_handlers_status(h)));
@@ -32,7 +32,7 @@ static void test_error() {
   ASSERT(!upb_ok(upb_handlers_status(h)));
   ASSERT(!upb_handlers_freeze(&h, 1, NULL));
 
-  // Clearing the error will let us proceed.
+  /* Clearing the error will let us proceed. */
   upb_handlers_clearerr(h);
   ASSERT(upb_handlers_freeze(&h, 1, NULL));
   ASSERT(upb_handlers_isfrozen(h));
diff --git a/tests/test_table.cc b/tests/test_table.cc
index 2cc5636..13b2330 100644
--- a/tests/test_table.cc
+++ b/tests/test_table.cc
@@ -62,6 +62,7 @@ void test_strtable(const vector<std::string>& keys, uint32_t num_to_insert) {
       upb_strtable_next(&iter)) {
     const char *key = upb_strtable_iter_key(&iter);
     std::string tmp(key, strlen(key));
+    ASSERT(strlen(key) == upb_strtable_iter_keylength(&iter));
     std::set<std::string>::iterator i = all.find(tmp);
     ASSERT(i != all.end());
     all.erase(i);
diff --git a/tests/test_util.h b/tests/test_util.h
index 27c2bb3..73a5c19 100644
--- a/tests/test_util.h
+++ b/tests/test_util.h
@@ -38,8 +38,8 @@ bool parse_buffer(upb::BytesSink* sink, void* subc, const char* buf,
     memcpy(buf2, buf + start, len);
 
     if (verbose) {
-      fprintf(stderr, "Calling parse(%zu) for bytes %zu-%zu of the input\n",
-              len, start, end);
+      fprintf(stderr, "Calling parse(%u) for bytes %u-%u of the input\n",
+              (unsigned)len, (unsigned)start, (unsigned)end);
     }
 
     size_t parsed = sink->PutBuffer(subc, buf2, len, &global_handle);
@@ -48,18 +48,18 @@ bool parse_buffer(upb::BytesSink* sink, void* subc, const char* buf,
     if (verbose) {
       if (parsed == len) {
         fprintf(stderr,
-                "parse(%zu) = %zu, complete byte count indicates success\n",
-                len, len);
+                "parse(%u) = %u, complete byte count indicates success\n",
+                (unsigned)len, (unsigned)len);
       } else if (parsed > len) {
         fprintf(stderr,
-                "parse(%zu) = %zu, long byte count indicates success and skip"
-                "of the next %zu bytes\n",
-                len, parsed, parsed - len);
+                "parse(%u) = %u, long byte count indicates success and skip"
+                "of the next %u bytes\n",
+                (unsigned)len, (unsigned)parsed, (unsigned)(parsed - len));
       } else {
         fprintf(stderr,
-                "parse(%zu) = %zu, short byte count indicates failure; "
-                "last %zu bytes were not consumed\n",
-                len, parsed, len - parsed);
+                "parse(%u) = %u, short byte count indicates failure; "
+                "last %u bytes were not consumed\n",
+                (unsigned)len, (unsigned)parsed, (unsigned)(len - parsed));
       }
     }
 
@@ -73,8 +73,8 @@ bool parse_buffer(upb::BytesSink* sink, void* subc, const char* buf,
                 "Error: decode function returned complete byte count but set "
                 "error status\n");
       }
-      fprintf(stderr, "Status: %s, parsed=%zu, len=%zu\n",
-              status->error_message(), parsed, len);
+      fprintf(stderr, "Status: %s, parsed=%u, len=%u\n",
+              status->error_message(), (unsigned)parsed, (unsigned)len);
       ASSERT(false);
     }