From fcd8889d5b68be6ca7d1df705ad2159777e2f379 Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Tue, 13 Jan 2015 18:14:39 -0800 Subject: Support oneofs in MRI Ruby C extension. --- ruby/tests/basic.rb | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) (limited to 'ruby/tests/basic.rb') diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb index 92655033..ceeaa8ad 100644 --- a/ruby/tests/basic.rb +++ b/ruby/tests/basic.rb @@ -73,6 +73,15 @@ module BasicTest optional :key, :string, 1 optional :value, :message, 2, "TestMessage2" end + + add_message "OneofMessage" do + oneof :my_oneof do + optional :a, :string, 1 + optional :b, :int32, 2 + optional :c, :message, 3, "TestMessage2" + optional :d, :enum, 4, "TestEnum" + end + end end TestMessage = pool.lookup("TestMessage").msgclass @@ -87,6 +96,7 @@ module BasicTest pool.lookup("MapMessageWireEquiv_entry1").msgclass MapMessageWireEquiv_entry2 = pool.lookup("MapMessageWireEquiv_entry2").msgclass + OneofMessage = pool.lookup("OneofMessage").msgclass # ------------ test cases --------------- @@ -582,6 +592,80 @@ module BasicTest "b" => TestMessage2.new(:foo => 2)} end + def test_oneof_descriptors + d = OneofMessage.descriptor + o = d.lookup_oneof("my_oneof") + assert o != nil + assert o.class == Google::Protobuf::OneofDescriptor + assert o.name == "my_oneof" + assert d.oneofs == [o] + assert o.count == 4 + field_names = o.map{|f| f.name}.sort + assert field_names == ["a", "b", "c", "d"] + end + + def test_oneof + d = OneofMessage.new + assert d.a == nil + assert d.b == nil + assert d.c == nil + assert d.d == nil + + d.a = "hi" + assert d.a == "hi" + assert d.b == nil + assert d.c == nil + assert d.d == nil + + d.b = 42 + assert d.a == nil + assert d.b == 42 + assert d.c == nil + assert d.d == nil + + d.c = TestMessage2.new(:foo => 100) + assert d.a == nil + assert d.b == nil + assert d.c.foo == 100 + assert d.d == nil + + d.d = :C + assert d.a == nil + assert d.b == nil + assert d.c == nil + assert d.d == :C + + d2 = OneofMessage.decode(OneofMessage.encode(d)) + assert d2 == d + + encoded_field_a = OneofMessage.encode(OneofMessage.new(:a => "string")) + encoded_field_b = OneofMessage.encode(OneofMessage.new(:b => 1000)) + encoded_field_c = OneofMessage.encode( + OneofMessage.new(:c => TestMessage2.new(:foo => 1))) + encoded_field_d = OneofMessage.encode(OneofMessage.new(:d => :B)) + + d3 = OneofMessage.decode( + encoded_field_c + encoded_field_a + encoded_field_d) + assert d3.a == nil + assert d3.b == nil + assert d3.c == nil + assert d3.d == :B + + d4 = OneofMessage.decode( + encoded_field_c + encoded_field_a + encoded_field_d + + encoded_field_c) + assert d4.a == nil + assert d4.b == nil + assert d4.c.foo == 1 + assert d4.d == nil + + d5 = OneofMessage.new(:a => "hello") + assert d5.a != nil + d5.a = nil + assert d5.a == nil + assert OneofMessage.encode(d5) == '' + end + def test_enum_field m = TestMessage.new assert m.optional_enum == :Default @@ -621,6 +705,14 @@ module BasicTest assert m.repeated_msg[0].object_id != m2.repeated_msg[0].object_id end + def test_eq + m = TestMessage.new(:optional_int32 => 42, + :repeated_int32 => [1, 2, 3]) + m2 = TestMessage.new(:optional_int32 => 43, + :repeated_int32 => [1, 2, 3]) + assert m != m2 + end + def test_enum_lookup assert TestEnum::A == 1 assert TestEnum::B == 2 -- cgit v1.2.3 From e1b7d38d9ad5dfd3719b1e9b1d588e08aba1afe8 Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Wed, 14 Jan 2015 17:14:05 -0800 Subject: Addressed code-review comments. --- ruby/ext/google/protobuf_c/defs.c | 14 +++--- ruby/ext/google/protobuf_c/encode_decode.c | 29 +++++++---- ruby/ext/google/protobuf_c/protobuf.h | 2 +- ruby/ext/google/protobuf_c/storage.c | 78 +++++++++++++++--------------- ruby/tests/basic.rb | 7 ++- 5 files changed, 71 insertions(+), 59 deletions(-) (limited to 'ruby/tests/basic.rb') diff --git a/ruby/ext/google/protobuf_c/defs.c b/ruby/ext/google/protobuf_c/defs.c index 371939ae..446a5586 100644 --- a/ruby/ext/google/protobuf_c/defs.c +++ b/ruby/ext/google/protobuf_c/defs.c @@ -287,7 +287,7 @@ void Descriptor_register(VALUE module) { rb_define_method(klass, "lookup", Descriptor_lookup, 1); rb_define_method(klass, "add_field", Descriptor_add_field, 1); rb_define_method(klass, "add_oneof", Descriptor_add_oneof, 1); - rb_define_method(klass, "oneofs", Descriptor_oneofs, 0); + rb_define_method(klass, "each_oneof", Descriptor_each_oneof, 0); rb_define_method(klass, "lookup_oneof", Descriptor_lookup_oneof, 1); rb_define_method(klass, "msgclass", Descriptor_msgclass, 0); rb_define_method(klass, "name", Descriptor_name, 0); @@ -409,23 +409,23 @@ VALUE Descriptor_add_oneof(VALUE _self, VALUE obj) { /* * call-seq: - * Descriptor.oneofs => list of OneofDescriptors + * Descriptor.each_oneof(&block) => nil * - * Returns a list of OneofDescriptors that are part of this message type. + * Invokes the given block for each oneof in this message type, passing the + * corresponding OneofDescriptor. */ -VALUE Descriptor_oneofs(VALUE _self) { +VALUE Descriptor_each_oneof(VALUE _self) { DEFINE_SELF(Descriptor, self, _self); - VALUE ret = rb_ary_new(); upb_msg_oneof_iter it; for (upb_msg_oneof_begin(&it, self->msgdef); !upb_msg_oneof_done(&it); upb_msg_oneof_next(&it)) { const upb_oneofdef* oneof = upb_msg_iter_oneof(&it); VALUE obj = get_def_obj(oneof); - rb_ary_push(ret, obj); + rb_yield(obj); } - return ret; + return Qnil; } /* diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c index 26a038b6..5e53b694 100644 --- a/ruby/ext/google/protobuf_c/encode_decode.c +++ b/ruby/ext/google/protobuf_c/encode_decode.c @@ -60,10 +60,10 @@ static const void *newsubmsghandlerdata(upb_handlers* h, uint32_t ofs, } typedef struct { - size_t ofs; // union data slot - size_t case_ofs; // oneof_case field - uint32_t tag; // tag number to place in data slot - const upb_msgdef *md; // msgdef, for oneof submessage handler + size_t ofs; // union data slot + size_t case_ofs; // oneof_case field + uint32_t oneof_case_num; // oneof-case number to place in oneof_case field + const upb_msgdef *md; // msgdef, for oneof submessage handler } oneof_handlerdata_t; static const void *newoneofhandlerdata(upb_handlers *h, @@ -73,7 +73,12 @@ static const void *newoneofhandlerdata(upb_handlers *h, oneof_handlerdata_t *hd = ALLOC(oneof_handlerdata_t); hd->ofs = ofs; hd->case_ofs = case_ofs; - hd->tag = upb_fielddef_number(f); + // We reuse the field tag number as a oneof union discriminant tag. Note that + // we don't expose these numbers to the user, so the only requirement is that + // we have some unique ID for each union case/possibility. The field tag + // numbers are already present and are easy to use so there's no reason to + // create a separate ID space. + hd->oneof_case_num = upb_fielddef_number(f); if (upb_fielddef_type(f) == UPB_TYPE_MESSAGE) { hd->md = upb_fielddef_msgsubdef(f); } else { @@ -294,7 +299,8 @@ static map_handlerdata_t* new_map_handlerdata( static bool oneof##type##_handler(void *closure, const void *hd, \ ctype val) { \ const oneof_handlerdata_t *oneofdata = hd; \ - DEREF(closure, oneofdata->case_ofs, uint32_t) = oneofdata->tag; \ + DEREF(closure, oneofdata->case_ofs, uint32_t) = \ + oneofdata->oneof_case_num; \ DEREF(closure, oneofdata->ofs, ctype) = val; \ return true; \ } @@ -317,7 +323,8 @@ static void *oneofstr_handler(void *closure, const oneof_handlerdata_t *oneofdata = hd; VALUE str = rb_str_new2(""); rb_enc_associate(str, kRubyStringUtf8Encoding); - DEREF(msg, oneofdata->case_ofs, uint32_t) = oneofdata->tag; + DEREF(msg, oneofdata->case_ofs, uint32_t) = + oneofdata->oneof_case_num; DEREF(msg, oneofdata->ofs, VALUE) = str; return (void*)str; } @@ -329,7 +336,8 @@ static void *oneofbytes_handler(void *closure, const oneof_handlerdata_t *oneofdata = hd; VALUE str = rb_str_new2(""); rb_enc_associate(str, kRubyString8bitEncoding); - DEREF(msg, oneofdata->case_ofs, uint32_t) = oneofdata->tag; + DEREF(msg, oneofdata->case_ofs, uint32_t) = + oneofdata->oneof_case_num; DEREF(msg, oneofdata->ofs, VALUE) = str; return (void*)str; } @@ -340,13 +348,14 @@ static void *oneofsubmsg_handler(void *closure, MessageHeader* msg = closure; const oneof_handlerdata_t *oneofdata = hd; uint32_t oldcase = DEREF(msg, oneofdata->case_ofs, uint32_t); - DEREF(msg, oneofdata->case_ofs, uint32_t) = oneofdata->tag; + DEREF(msg, oneofdata->case_ofs, uint32_t) = + oneofdata->oneof_case_num; VALUE subdesc = get_def_obj((void*)oneofdata->md); VALUE subklass = Descriptor_msgclass(subdesc); - if (oldcase != oneofdata->tag || + if (oldcase != oneofdata->oneof_case_num || DEREF(msg, oneofdata->ofs, VALUE) == Qnil) { DEREF(msg, oneofdata->ofs, VALUE) = rb_class_new_instance(0, NULL, subklass); diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h index 84f318b3..5b94ae26 100644 --- a/ruby/ext/google/protobuf_c/protobuf.h +++ b/ruby/ext/google/protobuf_c/protobuf.h @@ -190,7 +190,7 @@ VALUE Descriptor_each(VALUE _self); VALUE Descriptor_lookup(VALUE _self, VALUE name); VALUE Descriptor_add_field(VALUE _self, VALUE obj); VALUE Descriptor_add_oneof(VALUE _self, VALUE obj); -VALUE Descriptor_oneofs(VALUE _self); +VALUE Descriptor_each_oneof(VALUE _self); VALUE Descriptor_lookup_oneof(VALUE _self, VALUE name); VALUE Descriptor_msgclass(VALUE _self); extern const rb_data_type_t _Descriptor_type; diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c index d526679a..adb74394 100644 --- a/ruby/ext/google/protobuf_c/storage.c +++ b/ruby/ext/google/protobuf_c/storage.c @@ -473,13 +473,26 @@ VALUE field_type_class(const upb_fielddef* field) { return type_class; } +static void* slot_memory(MessageLayout* layout, + const void* storage, + const upb_fielddef* field) { + return ((uint8_t *)storage) + + layout->fields[upb_fielddef_index(field)].offset; +} + +static uint32_t* slot_oneof_case(MessageLayout* layout, + const void* storage, + const upb_fielddef* field) { + return (uint32_t *)(((uint8_t *)storage) + + layout->fields[upb_fielddef_index(field)].case_offset); +} + + VALUE layout_get(MessageLayout* layout, const void* storage, const upb_fielddef* field) { - void* memory = ((uint8_t *)storage) + - layout->fields[upb_fielddef_index(field)].offset; - uint32_t* oneof_case = (uint32_t *)(((uint8_t *)storage) + - layout->fields[upb_fielddef_index(field)].case_offset); + void* memory = slot_memory(layout, storage, field); + uint32_t* oneof_case = slot_oneof_case(layout, storage, field); if (upb_fielddef_containingoneof(field)) { if (*oneof_case != upb_fielddef_number(field)) { @@ -552,10 +565,8 @@ void layout_set(MessageLayout* layout, void* storage, const upb_fielddef* field, VALUE val) { - void* memory = ((uint8_t *)storage) + - layout->fields[upb_fielddef_index(field)].offset; - uint32_t* oneof_case = (uint32_t *)(((uint8_t *)storage) + - layout->fields[upb_fielddef_index(field)].case_offset); + void* memory = slot_memory(layout, storage, field); + uint32_t* oneof_case = slot_oneof_case(layout, storage, field); if (upb_fielddef_containingoneof(field)) { if (val == Qnil) { @@ -592,10 +603,8 @@ void layout_init(MessageLayout* layout, !upb_msg_field_done(&it); upb_msg_field_next(&it)) { const upb_fielddef* field = upb_msg_iter_field(&it); - void* memory = ((uint8_t *)storage) + - layout->fields[upb_fielddef_index(field)].offset; - uint32_t* oneof_case = (uint32_t *)(((uint8_t *)storage) + - layout->fields[upb_fielddef_index(field)].case_offset); + void* memory = slot_memory(layout, storage, field); + uint32_t* oneof_case = slot_oneof_case(layout, storage, field); if (upb_fielddef_containingoneof(field)) { memset(memory, 0, NATIVE_SLOT_MAX_SIZE); @@ -652,10 +661,8 @@ void layout_mark(MessageLayout* layout, void* storage) { !upb_msg_field_done(&it); upb_msg_field_next(&it)) { const upb_fielddef* field = upb_msg_iter_field(&it); - void* memory = ((uint8_t *)storage) + - layout->fields[upb_fielddef_index(field)].offset; - uint32_t* oneof_case = (uint32_t *)(((uint8_t *)storage) + - layout->fields[upb_fielddef_index(field)].case_offset); + void* memory = slot_memory(layout, storage, field); + uint32_t* oneof_case = slot_oneof_case(layout, storage, field); if (upb_fielddef_containingoneof(field)) { if (*oneof_case == upb_fielddef_number(field)) { @@ -675,14 +682,11 @@ void layout_dup(MessageLayout* layout, void* to, void* from) { !upb_msg_field_done(&it); upb_msg_field_next(&it)) { const upb_fielddef* field = upb_msg_iter_field(&it); - void* to_memory = ((uint8_t *)to) + - layout->fields[upb_fielddef_index(field)].offset; - uint32_t* to_oneof_case = (uint32_t *)(((uint8_t *)to) + - layout->fields[upb_fielddef_index(field)].case_offset); - void* from_memory = ((uint8_t *)from) + - layout->fields[upb_fielddef_index(field)].offset; - uint32_t* from_oneof_case = (uint32_t *)(((uint8_t *)from) + - layout->fields[upb_fielddef_index(field)].case_offset); + + void* to_memory = slot_memory(layout, to, field); + uint32_t* to_oneof_case = slot_oneof_case(layout, to, field); + void* from_memory = slot_memory(layout, from, field); + uint32_t* from_oneof_case = slot_oneof_case(layout, from, field); if (upb_fielddef_containingoneof(field)) { if (*from_oneof_case == upb_fielddef_number(field)) { @@ -705,14 +709,11 @@ void layout_deep_copy(MessageLayout* layout, void* to, void* from) { !upb_msg_field_done(&it); upb_msg_field_next(&it)) { const upb_fielddef* field = upb_msg_iter_field(&it); - void* to_memory = ((uint8_t *)to) + - layout->fields[upb_fielddef_index(field)].offset; - uint32_t* to_oneof_case = (uint32_t *)(((uint8_t *)to) + - layout->fields[upb_fielddef_index(field)].case_offset); - void* from_memory = ((uint8_t *)from) + - layout->fields[upb_fielddef_index(field)].offset; - uint32_t* from_oneof_case = (uint32_t *)(((uint8_t *)from) + - layout->fields[upb_fielddef_index(field)].case_offset); + + void* to_memory = slot_memory(layout, to, field); + uint32_t* to_oneof_case = slot_oneof_case(layout, to, field); + void* from_memory = slot_memory(layout, from, field); + uint32_t* from_oneof_case = slot_oneof_case(layout, from, field); if (upb_fielddef_containingoneof(field)) { if (*from_oneof_case == upb_fielddef_number(field)) { @@ -737,14 +738,11 @@ VALUE layout_eq(MessageLayout* layout, void* msg1, void* msg2) { !upb_msg_field_done(&it); upb_msg_field_next(&it)) { const upb_fielddef* field = upb_msg_iter_field(&it); - void* msg1_memory = ((uint8_t *)msg1) + - layout->fields[upb_fielddef_index(field)].offset; - uint32_t* msg1_oneof_case = (uint32_t *)(((uint8_t *)msg1) + - layout->fields[upb_fielddef_index(field)].case_offset); - void* msg2_memory = ((uint8_t *)msg2) + - layout->fields[upb_fielddef_index(field)].offset; - uint32_t* msg2_oneof_case = (uint32_t *)(((uint8_t *)msg2) + - layout->fields[upb_fielddef_index(field)].case_offset); + + void* msg1_memory = slot_memory(layout, msg1, field); + uint32_t* msg1_oneof_case = slot_oneof_case(layout, msg1, field); + void* msg2_memory = slot_memory(layout, msg2, field); + uint32_t* msg2_oneof_case = slot_oneof_case(layout, msg2, field); if (upb_fielddef_containingoneof(field)) { if (*msg1_oneof_case != *msg2_oneof_case || diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb index ceeaa8ad..3e99f3b0 100644 --- a/ruby/tests/basic.rb +++ b/ruby/tests/basic.rb @@ -598,7 +598,12 @@ module BasicTest assert o != nil assert o.class == Google::Protobuf::OneofDescriptor assert o.name == "my_oneof" - assert d.oneofs == [o] + oneof_count = 0 + d.each_oneof{ |oneof| + oneof_count += 1 + assert oneof == o + } + assert oneof_count == 1 assert o.count == 4 field_names = o.map{|f| f.name}.sort assert field_names == ["a", "b", "c", "d"] -- cgit v1.2.3 From e2debef5d8cd084946bd14fecabda5c328382114 Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Wed, 14 Jan 2015 18:02:27 -0800 Subject: Ruby extension: added oneof accessor. --- ruby/ext/google/protobuf_c/encode_decode.c | 3 +- ruby/ext/google/protobuf_c/message.c | 45 ++++++++++++++++++++++++++++++ ruby/tests/basic.rb | 6 ++++ 3 files changed, 53 insertions(+), 1 deletion(-) (limited to 'ruby/tests/basic.rb') diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c index 5e53b694..0db86209 100644 --- a/ruby/ext/google/protobuf_c/encode_decode.c +++ b/ruby/ext/google/protobuf_c/encode_decode.c @@ -77,7 +77,8 @@ static const void *newoneofhandlerdata(upb_handlers *h, // we don't expose these numbers to the user, so the only requirement is that // we have some unique ID for each union case/possibility. The field tag // numbers are already present and are easy to use so there's no reason to - // create a separate ID space. + // create a separate ID space. In addition, using the field tag number here + // lets us easily look up the field in the oneof accessor. hd->oneof_case_num = upb_fielddef_number(f); if (upb_fielddef_type(f) == UPB_TYPE_MESSAGE) { hd->md = upb_fielddef_msgsubdef(f); diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index ee8881d4..d0b09014 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/ruby/ext/google/protobuf_c/message.c @@ -70,6 +70,36 @@ VALUE Message_alloc(VALUE klass) { return ret; } +static VALUE which_oneof_field(MessageHeader* self, const upb_oneofdef* o) { + // If no fields in the oneof, always nil. + if (upb_oneofdef_numfields(o) == 0) { + return Qnil; + } + // Grab the first field in the oneof so we can get its layout info to find the + // oneof_case field. + upb_oneof_iter it; + upb_oneof_begin(&it, o); + assert(!upb_oneof_done(&it)); + const upb_fielddef* first_field = upb_oneof_iter_field(&it); + assert(upb_fielddef_containingoneof(first_field) != NULL); + + size_t case_ofs = + self->descriptor->layout-> + fields[upb_fielddef_index(first_field)].case_offset; + uint32_t oneof_case = *((uint32_t*)(Message_data(self) + case_ofs)); + + // oneof_case == 0 indicates no field set. + if (oneof_case == 0) { + return Qnil; + } + + // oneof_case is a field index, so find that field. + const upb_fielddef* f = upb_oneofdef_itof(o, oneof_case); + assert(f != NULL); + + return ID2SYM(rb_intern(upb_fielddef_name(f))); +} + /* * call-seq: * Message.method_missing(*args) @@ -82,6 +112,10 @@ VALUE Message_alloc(VALUE klass) { * * msg.foo = 42 * puts msg.foo + * + * This method also provides read-only accessors for oneofs. If a oneof exists + * with name 'my_oneof', then msg.my_oneof will return a Ruby symbol equal to + * the name of the field in that oneof that is currently set, or nil if none. */ VALUE Message_method_missing(int argc, VALUE* argv, VALUE _self) { MessageHeader* self; @@ -104,6 +138,17 @@ VALUE Message_method_missing(int argc, VALUE* argv, VALUE _self) { name_len--; } + // Check for a oneof name first. + const upb_oneofdef* o = upb_msgdef_ntoo(self->descriptor->msgdef, + name, name_len); + if (o != NULL) { + if (setter) { + rb_raise(rb_eRuntimeError, "Oneof accessors are read-only."); + } + return which_oneof_field(self, o); + } + + // Otherwise, check for a field with that name. const upb_fielddef* f = upb_msgdef_ntof(self->descriptor->msgdef, name, name_len); diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb index 3e99f3b0..1b7508bb 100644 --- a/ruby/tests/basic.rb +++ b/ruby/tests/basic.rb @@ -615,30 +615,35 @@ module BasicTest assert d.b == nil assert d.c == nil assert d.d == nil + assert d.my_oneof == nil d.a = "hi" assert d.a == "hi" assert d.b == nil assert d.c == nil assert d.d == nil + assert d.my_oneof == :a d.b = 42 assert d.a == nil assert d.b == 42 assert d.c == nil assert d.d == nil + assert d.my_oneof == :b d.c = TestMessage2.new(:foo => 100) assert d.a == nil assert d.b == nil assert d.c.foo == 100 assert d.d == nil + assert d.my_oneof == :c d.d = :C assert d.a == nil assert d.b == nil assert d.c == nil assert d.d == :C + assert d.my_oneof == :d d2 = OneofMessage.decode(OneofMessage.encode(d)) assert d2 == d @@ -669,6 +674,7 @@ module BasicTest d5.a = nil assert d5.a == nil assert OneofMessage.encode(d5) == '' + assert d5.my_oneof == nil end def test_enum_field -- cgit v1.2.3