Support for maps in the MRI C Ruby extension. #155
Conversation
This adds the Map container and support for parsing and serializing maps in the protobuf wire format (as defined by the C++ implementation, with MapEntry submessages in a repeated field). JSON map serialization/parsing are not yet supported as these will require some changes to upb as well.
|
@haberman to review. |
| * MessageBuilderContext.map(name, key_type, value_type, number, | ||
| * value_type_class = nil) | ||
| * | ||
| * Defines a new map field on this message type with the given key and value types, tag |
|
Thanks! Updated based on comments. |
| * type_class must be a string, if present (as accepted by | ||
| * FieldDescriptor#submsg_name=). | ||
| */ | ||
| VALUE MessageBuilderContext_map(int argc, VALUE* argv, VALUE _self) { |
There was a problem hiding this comment.
It strikes me that this function is the equivalent of 3 or 4 lines of Ruby code, plus the special call to set the new message type as a map entry. I wonder if there is a way to implement it this way?
I envision something like:
VALUE message_type = rb_eval(
"if key_type == :float or key_type == :double or key_type == :enum or key_type == :message\n"
" raise 'Cannot add a map...'\n"
"end\n"
"add_message :MapEntry {\n"
" optional key_type, :key, 1\n"
" optional value_type, :value, 2\n"
"}");
upb_msgdef_setmapentry(ruby_to_Descriptor(message_type), true);
Don't invest too much effort if this looks tricky, but if this were possible it would make the code a lot shorter, clearer, and more robust, since it would be expressed in terms of the DSL you already previous defined.
There was a problem hiding this comment.
I poked a bit at this but it seemed more complex and error-prone to construct a string of Ruby code than to just do the work in C. Totally agree that in principle it should be a simple layering, but I think in practice this is not too bad.
|
Addressed comments, thanks! |
|
|
||
| // Replace any existing value by issuing a 'remove' operation first. | ||
| upb_value oldv; | ||
| upb_strtable_remove2(&self->table, keyval, length, &oldv); |
There was a problem hiding this comment.
The last parameter can be NULL when you don't care about it.
|
|
||
| // For each member of other, check that a member exists at the same key in | ||
| // self. We don't need to compare values here -- if the key exists in both, we | ||
| // compared values above; if not, we already know that the maps are not equal. |
There was a problem hiding this comment.
Is this necessary? If the two maps are the same size and all keys in m1 are known to be in m2, how could there be a key in m2 not in m1?
There was a problem hiding this comment.
Excellent point! Done.
|
Addressed comments, thanks! |
|
|
||
| m = Google::Protobuf::Map.new(:string, :message, TestMessage, | ||
| { "a" => TestMessage.new(:optional_int32 => 42), | ||
| "b" => TestMessage.new(:optional_int32 => 84) }) |
There was a problem hiding this comment.
Done (and others in this file). Sorry about that!
Support for maps in the MRI C Ruby extension.
This adds the Map container and support for parsing and serializing maps
in the protobuf wire format (as defined by the C++ implementation, with
MapEntry submessages in a repeated field). JSON map
serialization/parsing are not yet supported as these will require some
changes to upb as well.