Skip to content
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

Ruby 2.7: Better support of Integer#[] with range arguments #2182

Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions spec/ruby/core/integer/element_reference_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -131,16 +131,10 @@
0b000001[-3, 4].should == 0b1000
end

it "ignores negative upper boundary" do
0b101001101[1..-1].should == 0b10100110
0b101001101[1..-2].should == 0b10100110
0b101001101[1..-3].should == 0b10100110
end

it "ignores upper boundary smaller than lower boundary" do
0b101001101[4..1].should == 0b10100
0b101001101[4..2].should == 0b10100
0b101001101[4..3].should == 0b10100
0b101001101[-4..-5].should == 0b1010011010000
end

it "raises FloatDomainError if any boundary is infinity" do
Expand Down
13 changes: 0 additions & 13 deletions spec/tags/core/integer/element_reference_tags.txt

This file was deleted.

53 changes: 51 additions & 2 deletions src/main/ruby/truffleruby/core/integer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
Object.deprecate_constant :Fixnum, :Bignum

class Integer < Numeric
FIXNUM_MAX = 0x3fffffff
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eregon not sure that it's safe to hardcode this value, but I did't find better solution, mb you know?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems odd to me we even need this value to implement Integer#[], do you know which arguments return it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this anymore.


# Have a copy in Integer of the Numeric version, as MRI does
alias_method :remainder, :remainder
Expand All @@ -59,9 +60,57 @@ def **(o)
redo_coerced :**, o
end

def [](index)
def [](index, len = undefined)
if index.kind_of?(Range)
handle_range(index)
else
handle_aref(index, len)
end
end

private def handle_aref(index, len)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't have extra methods on Integer, even if private.
We should move them to class methods on a new Truffle::IntegerOperations module.
I can do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed all the comments apart from this one, not sure what I can do here. More general question, trying to avoid creating extra private methods is dictated by how truffleruby treats such code or it's code style preference?

Copy link
Member

@eregon eregon Dec 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because a Ruby program might notice this, e.g. some Ruby program would defined a public Integer#handle_aref, then it could conflict. It's also visible via private_methods, send, etc.

index = Primitive.rb_to_int(index)
index < 0 ? 0 : (self >> index) & 1
if Primitive.undefined?(len)
index < 0 ? 0 : (self >> index) & 1
else
num = self >> index
mask = (1 << len) - 1

num & mask
end
end

private def handle_range(range)
validate_range(range)

if !Primitive.nil?(range.begin) && !Primitive.nil?(range.end)
len = range.end - range.begin
num = self >> range.begin
mask = mask(range, len)

range.end < range.begin ? num : num & mask
elsif Primitive.nil? range.end

return self >> range.begin
else
result = self & mask(range, range.end)
raise ArgumentError, 'The beginless range for Integer#[] results in infinity' if result != 0 && range.end >= 0

0
end
end

private def validate_range(index)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you inline this method?

raise FloatDomainError , 'Infinity' if index.begin == Float::INFINITY || index.end == Float::INFINITY
raise FloatDomainError , '-Infinity' if index.begin == -Float::INFINITY || index.end == -Float::INFINITY
end

private def mask(range, idx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this logic in the two callers, and e.g. do len += 1 unless range.exclude_end? so then it's just (1 << len) - 1?

if range.exclude_end?
(1 << idx) - 1
else
(1 << (idx +1)) - 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+ 1

end
end

def allbits?(mask)
Expand Down
3 changes: 1 addition & 2 deletions test/mri/excludes/TestInteger.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,4 @@
exclude :test_floor, "needs investigation"
exclude :test_pow, "needs investigation"
exclude :test_truncate, "needs investigation"
exclude :test_Integer_with_invalid_exception, "needs investigation"
exclude :test_aref, "needs investigation"
exclude :test_Integer_with_invalid_exception, "needs investigation"