Merge pull request #584 from haberman/cwarnings

Ruby: Conform to C89/C90 variable declaration rules.

ruby/ext/google/protobuf_c/map.c

@@ -169,8 +169,7 @@ VALUE Map_alloc(VALUE klass) {
   Map* self = ALLOC(Map);
   memset(self, 0, sizeof(Map));
   self->value_type_class = Qnil;
-  VALUE ret = TypedData_Wrap_Struct(klass, &Map_type, self);
-  return ret;
+  return TypedData_Wrap_Struct(klass, &Map_type, self);
 }
 
 static bool needs_typeclass(upb_fieldtype_t type) {
@@ -215,6 +214,7 @@ static bool needs_typeclass(upb_fieldtype_t type) {
  */
 VALUE Map_init(int argc, VALUE* argv, VALUE _self) {
   Map* self = ruby_to_Map(_self);
+  int init_value_arg;
 
   // We take either two args (:key_type, :value_type), three args (:key_type,
   // :value_type, "ValueMessageType"), or four args (the above plus an initial
@@ -241,7 +241,7 @@ VALUE Map_init(int argc, VALUE* argv, VALUE _self) {
       rb_raise(rb_eArgError, "Invalid key type for map.");
   }
 
-  int init_value_arg = 2;
+  init_value_arg = 2;
   if (needs_typeclass(self->value_type) && argc > 2) {
     self->value_type_class = argv[2];
     validate_type_class(self->value_type, self->value_type_class);
@@ -356,9 +356,9 @@ VALUE Map_index(VALUE _self, VALUE key) {
   char keybuf[TABLE_KEY_BUF_LENGTH];
   const char* keyval = NULL;
   size_t length = 0;
+  upb_value v;
   table_key(self, key, keybuf, &keyval, &length);
 
-  upb_value v;
   if (upb_strtable_lookup2(&self->table, keyval, length, &v)) {
     void* mem = value_memory(&v);
     return native_slot_get(self->value_type, self->value_type_class, mem);
@@ -381,10 +381,11 @@ VALUE Map_index_set(VALUE _self, VALUE key, VALUE value) {
   char keybuf[TABLE_KEY_BUF_LENGTH];
   const char* keyval = NULL;
   size_t length = 0;
+  upb_value v;
+  void* mem;
   table_key(self, key, keybuf, &keyval, &length);
 
-  upb_value v;
-  void* mem = value_memory(&v);
+  mem = value_memory(&v);
   native_slot_set(self->value_type, self->value_type_class, mem, value);
 
   // Replace any existing value by issuing a 'remove' operation first.
@@ -432,9 +433,9 @@ VALUE Map_delete(VALUE _self, VALUE key) {
   char keybuf[TABLE_KEY_BUF_LENGTH];
   const char* keyval = NULL;
   size_t length = 0;
+  upb_value v;
   table_key(self, key, keybuf, &keyval, &length);
 
-  upb_value v;
   if (upb_strtable_remove2(&self->table, keyval, length, &v)) {
     void* mem = value_memory(&v);
     return native_slot_get(self->value_type, self->value_type_class, mem);
@@ -564,6 +565,8 @@ VALUE Map_deep_copy(VALUE _self) {
  */
 VALUE Map_eq(VALUE _self, VALUE _other) {
   Map* self = ruby_to_Map(_self);
+  Map* other;
+  upb_strtable_iter it;
 
   // Allow comparisons to Ruby hashmaps by converting to a temporary Map
   // instance. Slow, but workable.
@@ -573,7 +576,7 @@ VALUE Map_eq(VALUE _self, VALUE _other) {
     _other = other_map;
   }
 
-  Map* other = ruby_to_Map(_other);
+  other = ruby_to_Map(_other);
 
   if (self == other) {
     return Qtrue;
@@ -589,7 +592,6 @@ VALUE Map_eq(VALUE _self, VALUE _other) {
 
   // For each member of self, check that an equal member exists at the same key
   // in other.
-  upb_strtable_iter it;
   for (upb_strtable_begin(&it, &self->table);
        !upb_strtable_done(&it);
        upb_strtable_next(&it)) {
@@ -719,6 +721,7 @@ VALUE Map_merge_into_self(VALUE _self, VALUE hashmap) {
 
     Map* self = ruby_to_Map(_self);
     Map* other = ruby_to_Map(hashmap);
+    upb_strtable_iter it;
 
     if (self->key_type != other->key_type ||
         self->value_type != other->value_type ||
@@ -726,19 +729,19 @@ VALUE Map_merge_into_self(VALUE _self, VALUE hashmap) {
       rb_raise(rb_eArgError, "Attempt to merge Map with mismatching types");
     }
 
-    upb_strtable_iter it;
     for (upb_strtable_begin(&it, &other->table);
          !upb_strtable_done(&it);
          upb_strtable_next(&it)) {
 
       // Replace any existing value by issuing a 'remove' operation first.
+      upb_value v;
       upb_value oldv;
       upb_strtable_remove2(&self->table,
                            upb_strtable_iter_key(&it),
                            upb_strtable_iter_keylength(&it),
                            &oldv);
 
-      upb_value v = upb_strtable_iter_value(&it);
+      v = upb_strtable_iter_value(&it);
       upb_strtable_insert2(&self->table,
                            upb_strtable_iter_key(&it),
                            upb_strtable_iter_keylength(&it),

ruby/ext/google/protobuf_c/protobuf.c

@@ -83,10 +83,11 @@ ID descriptor_instancevar_interned;
 // This must be named "Init_protobuf_c" because the Ruby module is named
 // "protobuf_c" -- the VM looks for this symbol in our .so.
 void Init_protobuf_c() {
-  descriptor_instancevar_interned = rb_intern(kDescriptorInstanceVar);
   VALUE google = rb_define_module("Google");
   VALUE protobuf = rb_define_module_under(google, "Protobuf");
   VALUE internal = rb_define_module_under(protobuf, "Internal");
+
+  descriptor_instancevar_interned = rb_intern(kDescriptorInstanceVar);
   DescriptorPool_register(protobuf);
   Descriptor_register(protobuf);
   FieldDescriptor_register(protobuf);

ruby/ext/google/protobuf_c/repeated_field.c

@@ -47,6 +47,10 @@ RepeatedField* ruby_to_RepeatedField(VALUE _self) {
   return self;
 }
 
+void* RepeatedField_memoryat(RepeatedField* self, int index, int element_size) {
+  return ((uint8_t *)self->elements) + index * element_size;
+}
+
 static int index_position(VALUE _index, RepeatedField* repeated_field) {
   int index = NUM2INT(_index);
   if (index < 0 && repeated_field->size > 0) {
@@ -113,16 +117,15 @@ VALUE RepeatedField_index(int argc, VALUE* argv, VALUE _self) {
   if (argc == 1){
     if (FIXNUM_P(arg)) {
       /* standard case */
+      void* memory;
       int index = index_position(argv[0], self);
       if (index < 0 || index >= self->size) {
         return Qnil;
       }
-      void* memory = (void *) (((uint8_t *)self->elements) +
-                               index * element_size);
+      memory = RepeatedField_memoryat(self, index, element_size);
       return native_slot_get(field_type, field_type_class, memory);
     }else{
       /* check if idx is Range */
-      size_t off;
       switch (rb_range_beg_len(arg, &beg, &len, self->size, 0)) {
         case Qfalse:
           break;
@@ -157,23 +160,24 @@ VALUE RepeatedField_index_set(VALUE _self, VALUE _index, VALUE val) {
   upb_fieldtype_t field_type = self->field_type;
   VALUE field_type_class = self->field_type_class;
   int element_size = native_slot_size(field_type);
+  void* memory;
 
   int index = index_position(_index, self);
   if (index < 0 || index >= (INT_MAX - 1)) {
     return Qnil;
   }
   if (index >= self->size) {
-    RepeatedField_reserve(self, index + 1);
     upb_fieldtype_t field_type = self->field_type;
     int element_size = native_slot_size(field_type);
+    RepeatedField_reserve(self, index + 1);
     for (int i = self->size; i <= index; i++) {
-      void* elem = (void *)(((uint8_t *)self->elements) + i * element_size);
+      void* elem = RepeatedField_memoryat(self, i, element_size);
       native_slot_init(field_type, elem);
     }
     self->size = index + 1;
   }
 
-  void* memory = (void *) (((uint8_t *)self->elements) + index * element_size);
+  memory = RepeatedField_memoryat(self, index, element_size);
   native_slot_set(field_type, field_type_class, memory, val);
   return Qnil;
 }
@@ -181,6 +185,8 @@ VALUE RepeatedField_index_set(VALUE _self, VALUE _index, VALUE val) {
 static int kInitialSize = 8;
 
 void RepeatedField_reserve(RepeatedField* self, int new_size) {
+  void* old_elems = self->elements;
+  int elem_size = native_slot_size(self->field_type);
   if (new_size <= self->capacity) {
     return;
   }
@@ -190,8 +196,6 @@ void RepeatedField_reserve(RepeatedField* self, int new_size) {
   while (self->capacity < new_size) {
     self->capacity *= 2;
   }
-  void* old_elems = self->elements;
-  int elem_size = native_slot_size(self->field_type);
   self->elements = ALLOC_N(uint8_t, elem_size * self->capacity);
   if (old_elems != NULL) {
     memcpy(self->elements, old_elems, self->size * elem_size);
@@ -209,11 +213,12 @@ VALUE RepeatedField_push(VALUE _self, VALUE val) {
   RepeatedField* self = ruby_to_RepeatedField(_self);
   upb_fieldtype_t field_type = self->field_type;
   int element_size = native_slot_size(field_type);
+  void* memory;
+
   RepeatedField_reserve(self, self->size + 1);
-  int index = self->size;
-  void* memory = (void *) (((uint8_t *)self->elements) + index * element_size);
+  memory = (void *) (((uint8_t *)self->elements) + self->size * element_size);
   native_slot_set(field_type, self->field_type_class, memory, val);
-  // native_slot_set may raise an error; bump index only after set.
+  // native_slot_set may raise an error; bump size only after set.
   self->size++;
   return _self;
 }
@@ -224,9 +229,10 @@ void RepeatedField_push_native(VALUE _self, void* data) {
   RepeatedField* self = ruby_to_RepeatedField(_self);
   upb_fieldtype_t field_type = self->field_type;
   int element_size = native_slot_size(field_type);
+  void* memory;
+
   RepeatedField_reserve(self, self->size + 1);
-  int index = self->size;
-  void* memory = (void *) (((uint8_t *)self->elements) + index * element_size);
+  memory = (void *) (((uint8_t *)self->elements) + self->size * element_size);
   memcpy(memory, data, element_size);
   self->size++;
 }
@@ -235,7 +241,7 @@ void* RepeatedField_index_native(VALUE _self, int index) {
   RepeatedField* self = ruby_to_RepeatedField(_self);
   upb_fieldtype_t field_type = self->field_type;
   int element_size = native_slot_size(field_type);
-  return ((uint8_t *)self->elements) + index * element_size;
+  return RepeatedField_memoryat(self, index, element_size);
 }
 
 /*
@@ -246,12 +252,16 @@ VALUE RepeatedField_pop_one(VALUE _self) {
   upb_fieldtype_t field_type = self->field_type;
   VALUE field_type_class = self->field_type_class;
   int element_size = native_slot_size(field_type);
+  int index;
+  void* memory;
+  VALUE ret;
+
   if (self->size == 0) {
     return Qnil;
   }
-  int index = self->size - 1;
-  void* memory = (void *) (((uint8_t *)self->elements) + index * element_size);
-  VALUE ret = native_slot_get(field_type, field_type_class, memory);
+  index = self->size - 1;
+  memory = RepeatedField_memoryat(self, index, element_size);
+  ret = native_slot_get(field_type, field_type_class, memory);
   self->size--;
   return ret;
 }
@@ -320,10 +330,10 @@ VALUE RepeatedField_dup(VALUE _self) {
   RepeatedField* self = ruby_to_RepeatedField(_self);
   VALUE new_rptfield = RepeatedField_new_this_type(_self);
   RepeatedField* new_rptfield_self = ruby_to_RepeatedField(new_rptfield);
-  RepeatedField_reserve(new_rptfield_self, self->size);
   upb_fieldtype_t field_type = self->field_type;
   size_t elem_size = native_slot_size(field_type);
   size_t off = 0;
+  RepeatedField_reserve(new_rptfield_self, self->size);
   for (int i = 0; i < self->size; i++, off += elem_size) {
     void* to_mem = (uint8_t *)new_rptfield_self->elements + off;
     void* from_mem = (uint8_t *)self->elements + off;
@@ -339,10 +349,10 @@ VALUE RepeatedField_deep_copy(VALUE _self) {
   RepeatedField* self = ruby_to_RepeatedField(_self);
   VALUE new_rptfield = RepeatedField_new_this_type(_self);
   RepeatedField* new_rptfield_self = ruby_to_RepeatedField(new_rptfield);
-  RepeatedField_reserve(new_rptfield_self, self->size);
   upb_fieldtype_t field_type = self->field_type;
   size_t elem_size = native_slot_size(field_type);
   size_t off = 0;
+  RepeatedField_reserve(new_rptfield_self, self->size);
   for (int i = 0; i < self->size; i++, off += elem_size) {
     void* to_mem = (uint8_t *)new_rptfield_self->elements + off;
     void* from_mem = (uint8_t *)self->elements + off;
@@ -389,34 +399,39 @@ VALUE RepeatedField_to_ary(VALUE _self) {
  * indicated that every element has equal value.
  */
 VALUE RepeatedField_eq(VALUE _self, VALUE _other) {
+  RepeatedField* self;
+  RepeatedField* other;
+
   if (_self == _other) {
     return Qtrue;
   }
-  RepeatedField* self = ruby_to_RepeatedField(_self);
 
   if (TYPE(_other) == T_ARRAY) {
     VALUE self_ary = RepeatedField_to_ary(_self);
     return rb_equal(self_ary, _other);
   }
 
-  RepeatedField* other = ruby_to_RepeatedField(_other);
+  self = ruby_to_RepeatedField(_self);
+  other = ruby_to_RepeatedField(_other);
   if (self->field_type != other->field_type ||
       self->field_type_class != other->field_type_class ||
       self->size != other->size) {
     return Qfalse;
   }
 
-  upb_fieldtype_t field_type = self->field_type;
-  size_t elem_size = native_slot_size(field_type);
-  size_t off = 0;
-  for (int i = 0; i < self->size; i++, off += elem_size) {
-    void* self_mem = ((uint8_t *)self->elements) + off;
-    void* other_mem = ((uint8_t *)other->elements) + off;
-    if (!native_slot_eq(field_type, self_mem, other_mem)) {
-      return Qfalse;
+  {
+    upb_fieldtype_t field_type = self->field_type;
+    size_t elem_size = native_slot_size(field_type);
+    size_t off = 0;
+    for (int i = 0; i < self->size; i++, off += elem_size) {
+      void* self_mem = ((uint8_t *)self->elements) + off;
+      void* other_mem = ((uint8_t *)other->elements) + off;
+      if (!native_slot_eq(field_type, self_mem, other_mem)) {
+        return Qfalse;
+      }
     }
+    return Qtrue;
   }
-  return Qtrue;
 }
 
 /*
@@ -488,7 +503,6 @@ VALUE RepeatedField_plus(VALUE _self, VALUE list) {
  * concats the passed in array to self.  Returns a Ruby array.
  */
 VALUE RepeatedField_concat(VALUE _self, VALUE list) {
-  RepeatedField* self = ruby_to_RepeatedField(_self);
   Check_Type(list, T_ARRAY);
   for (int i = 0; i < RARRAY_LEN(list); i++) {
     RepeatedField_push(_self, rb_ary_entry(list, i));
@@ -564,9 +578,9 @@ void RepeatedField_init_args(int argc, VALUE* argv,
 
 void RepeatedField_mark(void* _self) {
   RepeatedField* self = (RepeatedField*)_self;
-  rb_gc_mark(self->field_type_class);
   upb_fieldtype_t field_type = self->field_type;
   int element_size = native_slot_size(field_type);
+  rb_gc_mark(self->field_type_class);
   for (int i = 0; i < self->size; i++) {
     void* memory = (((uint8_t *)self->elements) + i * element_size);
     native_slot_mark(self->field_type, memory);
@@ -597,8 +611,7 @@ VALUE RepeatedField_alloc(VALUE klass) {
   self->capacity = 0;
   self->field_type = -1;
   self->field_type_class = Qnil;
-  VALUE ret = TypedData_Wrap_Struct(klass, &RepeatedField_type, self);
-  return ret;
+  return TypedData_Wrap_Struct(klass, &RepeatedField_type, self);
 }
 
 VALUE RepeatedField_init(int argc, VALUE* argv, VALUE self) {

ruby/ext/google/protobuf_c/storage.c

@@ -166,11 +166,11 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class,
       break;
     }
     case UPB_TYPE_ENUM: {
+      int32_t int_val = 0;
       if (!is_ruby_num(value) && TYPE(value) != T_SYMBOL) {
         rb_raise(rb_eTypeError,
                  "Expected number or symbol type for enum field.");
       }
-      int32_t int_val = 0;
       if (TYPE(value) == T_SYMBOL) {
         // Ensure that the given symbol exists in the enum module.
         VALUE lookup = rb_funcall(type_class, rb_intern("resolve"), 1, value);
@@ -346,24 +346,33 @@ bool native_slot_eq(upb_fieldtype_t type, void* mem1, void* mem2) {
 // Map field utilities.
 // -----------------------------------------------------------------------------
 
-bool is_map_field(const upb_fielddef* field) {
+const upb_msgdef* tryget_map_entry_msgdef(const upb_fielddef* field) {
+  const upb_msgdef* subdef;
   if (upb_fielddef_label(field) != UPB_LABEL_REPEATED ||
       upb_fielddef_type(field) != UPB_TYPE_MESSAGE) {
-    return false;
+    return NULL;
   }
-  const upb_msgdef* subdef = upb_fielddef_msgsubdef(field);
-  return upb_msgdef_mapentry(subdef);
+  subdef = upb_fielddef_msgsubdef(field);
+  return upb_msgdef_mapentry(subdef) ? subdef : NULL;
+}
+
+const upb_msgdef *map_entry_msgdef(const upb_fielddef* field) {
+  const upb_msgdef* subdef = tryget_map_entry_msgdef(field);
+  assert(subdef);
+  return subdef;
+}
+
+bool is_map_field(const upb_fielddef *field) {
+  return tryget_map_entry_msgdef(field) != NULL;
 }
 
 const upb_fielddef* map_field_key(const upb_fielddef* field) {
-  assert(is_map_field(field));
-  const upb_msgdef* subdef = upb_fielddef_msgsubdef(field);
+  const upb_msgdef* subdef = map_entry_msgdef(field);
   return map_entry_key(subdef);
 }
 
 const upb_fielddef* map_field_value(const upb_fielddef* field) {
-  assert(is_map_field(field));
-  const upb_msgdef* subdef = upb_fielddef_msgsubdef(field);
+  const upb_msgdef* subdef = map_entry_msgdef(field);
   return map_entry_value(subdef);
 }
 
@@ -391,14 +400,17 @@ static size_t align_up_to(size_t offset, size_t granularity) {
 MessageLayout* create_layout(const upb_msgdef* msgdef) {
   MessageLayout* layout = ALLOC(MessageLayout);
   int nfields = upb_msgdef_numfields(msgdef);
-  layout->fields = ALLOC_N(MessageField, nfields);
-
   upb_msg_field_iter it;
+  upb_msg_oneof_iter oit;
   size_t off = 0;
+
+  layout->fields = ALLOC_N(MessageField, nfields);
+
   for (upb_msg_field_begin(&it, msgdef);
        !upb_msg_field_done(&it);
        upb_msg_field_next(&it)) {
     const upb_fielddef* field = upb_msg_iter_field(&it);
+    size_t field_size;
 
     if (upb_fielddef_containingoneof(field)) {
       // Oneofs are handled separately below.
@@ -406,7 +418,7 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) {
     }
 
     // Allocate |field_size| bytes for this field in the layout.
-    size_t field_size = 0;
+    field_size = 0;
     if (upb_fielddef_label(field) == UPB_LABEL_REPEATED) {
       field_size = sizeof(VALUE);
     } else {
@@ -433,11 +445,11 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) {
   // members (8 or 16 bits respectively), so conceivably we could assign
   // consecutive case numbers and then pick a smaller oneof case slot size, but
   // the complexity to implement this indirection is probably not worthwhile.
-  upb_msg_oneof_iter oit;
   for (upb_msg_oneof_begin(&oit, msgdef);
        !upb_msg_oneof_done(&oit);
        upb_msg_oneof_next(&oit)) {
     const upb_oneofdef* oneof = upb_msg_iter_oneof(&oit);
+    upb_oneof_iter fit;
 
     // Always allocate NATIVE_SLOT_MAX_SIZE bytes, but share the slot between
     // all fields.
@@ -445,7 +457,6 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) {
     // Align the offset.
     off = align_up_to(off, field_size);
     // Assign all fields in the oneof this same offset.
-    upb_oneof_iter fit;
     for (upb_oneof_begin(&fit, oneof);
          !upb_oneof_done(&fit);
          upb_oneof_next(&fit)) {
@@ -460,12 +471,12 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) {
        !upb_msg_oneof_done(&oit);
        upb_msg_oneof_next(&oit)) {
     const upb_oneofdef* oneof = upb_msg_iter_oneof(&oit);
+    upb_oneof_iter fit;
 
     size_t field_size = sizeof(uint32_t);
     // Align the offset.
     off = (off + field_size - 1) & ~(field_size - 1);
     // Assign all fields in the oneof this same offset.
-    upb_oneof_iter fit;
     for (upb_oneof_begin(&fit, oneof);
          !upb_oneof_done(&fit);
          upb_oneof_next(&fit)) {
@@ -541,6 +552,7 @@ VALUE layout_get(MessageLayout* layout,
 }
 
 static void check_repeated_field_type(VALUE val, const upb_fielddef* field) {
+  RepeatedField* self;
   assert(upb_fielddef_label(field) == UPB_LABEL_REPEATED);
 
   if (!RB_TYPE_P(val, T_DATA) || !RTYPEDDATA_P(val) ||
@@ -548,7 +560,7 @@ static void check_repeated_field_type(VALUE val, const upb_fielddef* field) {
     rb_raise(rb_eTypeError, "Expected repeated field array");
   }
 
-  RepeatedField* self = ruby_to_RepeatedField(val);
+  self = ruby_to_RepeatedField(val);
   if (self->field_type != upb_fielddef_type(field)) {
     rb_raise(rb_eTypeError, "Repeated field array has wrong element type");
   }
@@ -564,16 +576,16 @@ static void check_repeated_field_type(VALUE val, const upb_fielddef* field) {
 }
 
 static void check_map_field_type(VALUE val, const upb_fielddef* field) {
-  assert(is_map_field(field));
   const upb_fielddef* key_field = map_field_key(field);
   const upb_fielddef* value_field = map_field_value(field);
+  Map* self;
 
   if (!RB_TYPE_P(val, T_DATA) || !RTYPEDDATA_P(val) ||
       RTYPEDDATA_TYPE(val) != &Map_type) {
     rb_raise(rb_eTypeError, "Expected Map instance");
   }
 
-  Map* self = ruby_to_Map(val);
+  self = ruby_to_Map(val);
   if (self->key_type != upb_fielddef_type(key_field)) {
     rb_raise(rb_eTypeError, "Map key type does not match field's key type");
   }