[
https://issues.apache.org/jira/browse/THRIFT-332?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12683337#action_12683337
]
Kevin Clark edited comment on THRIFT-332 at 3/18/09 11:23 PM:
--------------------------------------------------------------
In general this looks great. Good work.
re: sets and maps don't hash well
Maybe you could iterate through the structs and compare by hand? It's ugly
though. Should probably try to find a way to fix this.
lib/rb/lib/thrift/transport.rb.orig is in the patch
I'm getting a failure I don't see in trunk:
{quote}
NameError in 'deprecate_module! should skip thrift library code when printing
caller'
undefined method `struct_fields' for module `Thrift::Struct'
./spec/../lib/thrift/deprecation.rb:98:in `instance_method'
./spec/../lib/thrift/deprecation.rb:98:in `method_missing'
./spec/../lib/thrift/struct.rb:41:in `fields_with_default_values'
./spec/../lib/thrift/deprecation.rb:98:in `call'
./spec/../lib/thrift/deprecation.rb:98:in `method_missing'
./spec/../lib/thrift/struct.rb:8:in `initialize'
./spec/deprecation_spec.rb:441:in `new'
./spec/deprecation_spec.rb:441:
{quote}
Maybe caused by this?
{quote}
-- # Obsoleted by THRIFT-246, which generates this method inline
-- # TODO: Should be removed at some point. -- Kevin Clark
-- def struct_fields
-- self.class.const_get(:FIELDS)
-- end
{quote}
But I appear to be regenerating the stuff in gen-rb, so I don't know what's up.
compact_protocol.rb:
In write_double:
{[email protected]([dub].pack("G").reverse) #TODO: make sure this is
right{quote}
Is it right?
compact_protocol.c:
The 5 defines at the top also show up in binary_protocol_accelerated. Could we
move them into a .h?
Is this going to hook into the native protocol hooks you wrote? If so, doesn't
rb_thrift_compact_proto_native_qmark need to return true? Looks like the native
hooks are commented out.
I guess this is a general issue, but do we have a way to test both the native
and non-native bindings?
In rb_thrift_compact_proto_read_message_begin, the max length of buf for
PROTOCOL_ID is 51 and for VERSION is 50(string.gsub('%d', '4294967296').length
is 49 + 1 for zero termination). Make sure to null out the last byte, or
rb_str_new2 could include garbage.
Do you need the memcpy in rb_thrift_compact_proto_read_double? Can you just
cast instead?
{quote}return rb_float_new((double)RSTRING(bytes)->ptr){quote}
was (Author: kclark):
In general this looks great. Good work.
re: sets and maps don't hash well
Maybe you could iterate through the structs and compare by hand? It's ugly
though. Should probably try to find a way to fix this.
lib/rb/lib/thrift/transport.rb.orig is in the patch
I'm getting a failure I don't see in trunk:
{monospace}
NameError in 'deprecate_module! should skip thrift library code when printing
caller'
undefined method `struct_fields' for module `Thrift::Struct'
./spec/../lib/thrift/deprecation.rb:98:in `instance_method'
./spec/../lib/thrift/deprecation.rb:98:in `method_missing'
./spec/../lib/thrift/struct.rb:41:in `fields_with_default_values'
./spec/../lib/thrift/deprecation.rb:98:in `call'
./spec/../lib/thrift/deprecation.rb:98:in `method_missing'
./spec/../lib/thrift/struct.rb:8:in `initialize'
./spec/deprecation_spec.rb:441:in `new'
./spec/deprecation_spec.rb:441:
{monospace}
Maybe caused by this?
- # Obsoleted by THRIFT-246, which generates this method inline
- # TODO: Should be removed at some point. -- Kevin Clark
- def struct_fields
- self.class.const_get(:FIELDS)
- end
But I appear to be regenerating the stuff in gen-rb, so I don't know what's up.
compact_protocol.rb:
In write_double:
@trans.write([dub].pack("G").reverse) #TODO: make sure this is right
Is it right?
compact_protocol.c:
The 5 defines at the top also show up in binary_protocol_accelerated. Could we
move them into a .h?
Is this going to hook into the native protocol hooks you wrote? If so, doesn't
rb_thrift_compact_proto_native_qmark need to return true? Looks like the native
hooks are commented out.
I guess this is a general issue, but do we have a way to test both the native
and non-native bindings?
In rb_thrift_compact_proto_read_message_begin, the max length of buf for
PROTOCOL_ID is 51 and for VERSION is 50(string.gsub('%d', '4294967296').length
is 49 + 1 for zero termination). Make sure to null out the last byte, or
rb_str_new2 could include garbage.
Do you need the memcpy in rb_thrift_compact_proto_read_double? Can you just
cast instead? return rb_float_new((double)RSTRING(bytes)->ptr)
> Compact Protocol in Ruby
> ------------------------
>
> Key: THRIFT-332
> URL: https://issues.apache.org/jira/browse/THRIFT-332
> Project: Thrift
> Issue Type: Sub-task
> Components: Library (Ruby)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Trivial
> Attachments: thrift-332-v2.patch, thrift-332-v3.patch,
> thrift-332.patch
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
