-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Abrandoned/decoding setters without copy #297
base: master
Are you sure you want to change the base?
Changes from all commits
22909fb
7637d60
e06f642
d096a27
0a9bfbb
481e730
476844f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -176,16 +176,18 @@ def define_accessor | |
else | ||
define_getter | ||
define_setter | ||
define_decode_setter | ||
end | ||
end | ||
|
||
def define_array_getter | ||
field = self | ||
field_name = field.name | ||
method_name = field.getter | ||
|
||
message_class.class_eval do | ||
define_method(method_name) do | ||
@values[field.name] ||= ::Protobuf::Field::FieldArray.new(field) | ||
@values[field_name] ||= ::Protobuf::Field::FieldArray.new(field) | ||
end | ||
end | ||
|
||
|
@@ -194,6 +196,7 @@ def define_array_getter | |
|
||
def define_array_setter | ||
field = self | ||
field_name = field.name | ||
method_name = field.setter | ||
|
||
message_class.class_eval do | ||
|
@@ -205,15 +208,15 @@ def define_array_setter | |
else | ||
fail TypeError, <<-TYPE_ERROR | ||
Expected repeated value of type '#{field.type_class}' | ||
Got '#{val.class}' for repeated protobuf field #{field.name} | ||
Got '#{val.class}' for repeated protobuf field #{field_name} | ||
TYPE_ERROR | ||
end | ||
|
||
if val.nil? || (val.respond_to?(:empty?) && val.empty?) | ||
@values.delete(field.name) | ||
@values.delete(field_name) | ||
else | ||
@values[field.name] ||= ::Protobuf::Field::FieldArray.new(field) | ||
@values[field.name].replace(val) | ||
@values[field_name] ||= ::Protobuf::Field::FieldArray.new(field) | ||
@values[field_name].replace(val) | ||
end | ||
end | ||
end | ||
|
@@ -234,17 +237,22 @@ def define_getter | |
::Protobuf.field_deprecator.deprecate_method(message_class, method_name) if field.deprecated? | ||
end | ||
|
||
def define_decode_setter | ||
# empty for now | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only a few variants of this method. Let's bring the duplicated common implementation here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably best, I wrote this while I was testing the string copy only and now need to refactor since a common pattern has emerged There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
end | ||
|
||
def define_setter | ||
field = self | ||
field_name = field.name | ||
method_name = field.setter | ||
|
||
message_class.class_eval do | ||
define_method(method_name) do |val| | ||
@encode = nil | ||
if val.nil? || (val.respond_to?(:empty?) && val.empty?) | ||
@values.delete(field.name) | ||
@values.delete(field_name) | ||
elsif field.acceptable?(val) | ||
@values[field.name] = field.coerce!(val) | ||
@values[field_name] = field.coerce!(val) | ||
else | ||
fail TypeError, "Unacceptable value #{val} for field #{field.name} of type #{field.type_class}" | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,19 @@ def decode(value) | |
value == 1 | ||
end | ||
|
||
def define_decode_setter | ||
field = self | ||
field_name = field.name | ||
tag_method_name = "_protobuf_decode_setter_#{field.tag}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make this private, |
||
|
||
message_class.class_eval do | ||
define_method(tag_method_name) do |val| | ||
@encode = nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
@values[field_name] = field.decode(val) | ||
end | ||
end | ||
end | ||
|
||
def encode(value) | ||
[value ? 1 : 0].pack('C') | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,7 @@ def self.encode(value, use_cache = true) | |
# | ||
|
||
def acceptable?(val) | ||
return true if val.is_a?(Integer) && val >= 0 && val < INT32_MAX | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we still want to compare to I understand wanting to avoid Just an idea def acceptable?(val)
int_val = val.is_a?(Integer)
val
else
coerce!(val)
end
int_val >= self.class.min && int_val <= self.class.max
end There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we do want to compare the max (which happens after this if it is larger than the smallest max); the smallest max is the INT32_MAX, so if it passes it (and is an Integer; which is the most common case, especially in deserialization) then we just set it instead of taking the long route (which in our usage is very uncommon) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess as a reader of this patch, I'm having a hard following what it's supposed to do. I didn't realize it would fall through to the old case. Maybe we could do something like this (comments included) for posterity? def acceptable?(val)
int_val = if val.is_a?(Integer)
return true if val >= 0 && val < INT32_MAX # return quickly for smallest integer size, hot code path
val
else
coerce!(val)
end
int_val >= self.class.min && int_val <= self.class.max
end |
||
int_val = coerce!(val) | ||
int_val >= self.class.min && int_val <= self.class.max | ||
rescue | ||
|
@@ -63,6 +64,19 @@ def coerce!(val) | |
Integer(val, 10) | ||
end | ||
|
||
def define_decode_setter | ||
field = self | ||
field_name = field.name | ||
tag_method_name = "_protobuf_decode_setter_#{field.tag}" | ||
|
||
message_class.class_eval do | ||
define_method(tag_method_name) do |val| | ||
@encode = nil | ||
@values[field_name] = val | ||
end | ||
end | ||
end | ||
|
||
def decode(value) | ||
value | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,14 +77,13 @@ def each_field | |
return to_enum(:each_field) unless block_given? | ||
|
||
self.class.all_fields.each do |field| | ||
value = __send__(field.getter) | ||
yield(field, value) | ||
yield(field, __send__(field.getter)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After #302 it would be the style to use |
||
end | ||
end | ||
|
||
def each_field_for_serialization | ||
self.class.all_fields.each do |field| | ||
value = @values[field.getter] | ||
value = @values[field.name] | ||
if value.nil? | ||
fail ::Protobuf::SerializationError, "Required field #{self.class.name}##{field.name} does not have a value." if field.required? | ||
next | ||
|
@@ -146,18 +145,16 @@ def ==(other) | |
end | ||
|
||
def [](name) | ||
if (field = self.class.get_field(name, true)) | ||
__send__(field.getter) | ||
end | ||
__send__(name) if respond_to?(name) | ||
end | ||
|
||
def []=(name, value) | ||
if (field = self.class.get_field(name, true)) | ||
__send__(field.setter, value) unless value.nil? | ||
setter = "#{name}=" | ||
|
||
if respond_to?(setter) | ||
__send__(setter, value) unless value.nil? | ||
else | ||
unless ::Protobuf.ignore_unknown_fields? | ||
fail ::Protobuf::FieldNotDefinedError, name | ||
end | ||
fail ::Protobuf::FieldNotDefinedError, name unless ::Protobuf.ignore_unknown_fields? | ||
end | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,12 @@ | ||
module Protobuf | ||
module VarintPure | ||
def decode(stream) | ||
value = index = 0 | ||
value = shift = 0 | ||
begin | ||
byte = stream.readbyte | ||
value |= (byte & 0x7f) << (7 * index) | ||
index += 1 | ||
end while (byte & 0x80).nonzero? | ||
value |= (byte & 127) << shift | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bikeshedding a little, but honestly I prefer the hex. Hex makes it easier to picture the individual bits in my head. |
||
shift += 7 | ||
end while byte > 127 | ||
value | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up that a lot of this work will need to be moved to the
[]=
and[]
methods after PR #302 is merged. In fact a lot of the setter/getter code in this PR will conflict, I comment on just a little of it below.