[
https://issues.apache.org/jira/browse/THRIFT-697?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12834099#action_12834099
]
Kevin Clark commented on THRIFT-697:
------------------------------------
Generally looks fine, but could use some cleanup. Some of this is nitpicky, so
take it with a grain of salt. My notes follow.
struct.c
There's a dangling TODO (to raise an exception) in rb_thrift_union_read and
another in _write. For read, we should throw and remove the TODO. For write, it
looks like it comes down to a .to_sym call in the Ruby. Maybe it'd be worth
benchmarking?
union.rb
If you think the current version is inelegant, I think you can rewrite
Thrift::Union#initialize as:
def initialize(name = nil, value = nil)
if name && value.nil?
raise Exception ..
end
if name
Thrift.check_type ...
@field_id = name_to_id(name.to_s)
end
@setfield = name
@value = value
end
It avoids an extra call to name_to_id (and the .to_s) and reads a little
cleaner.
You may want to benchmark if there's a difference in performance for uses of
define_method vs class eval. I thought I remember there being a significant
difference, but that might not be the case anymore.
Why the handwritten getter/setter (and then a protected version of the exact
same thing)? Feels very un-ruby
Is the 'set' required used anywhere?
struct_union.rb
I know it was moved from before, but is there a reason each_field needs to
exist? Seems like a needless sort/copy/method call, but maybe there's a reason
we were doing this that I've forgotten? If it does need to exist, is there a
reason name_to_ids doesn't use it?
union_spec.rb
Is there a reason you're testing that .new does't return nil? I'm not sure how
to make that fail.
t_rb_generator.cc
Looks like there might be some spacing issues with the if in generate_struct
(if tstruct->is_union)
In generate_union_accessors, I'd probably move the declaration of m_iter into
the for, or atleast into the if clause, but it's really nitpicky and I don't
have an issue with it staying that way.
If we're going to do a copy (from f_iter to field) in
generate_rb_union_validatior, might as well declare field as const.
> Union support in Ruby
> ---------------------
>
> Key: THRIFT-697
> URL: https://issues.apache.org/jira/browse/THRIFT-697
> Project: Thrift
> Issue Type: New Feature
> Components: Compiler (Ruby), Library (Ruby)
> Affects Versions: 0.3
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Fix For: 0.3
>
> Attachments: thrift-697.patch
>
>
> We'd like to have direct native support for Union-style structs in Ruby.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
