Skip to content

Fix BigDecimal#to_big_i regression #8790

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.

Sign up for GitHub

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

Merged
straight-shoota merged 1 commit into from
Feb 18, 2020
Merged

Fix BigDecimal#to_big_i regression #8790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 20 additions & 0 deletions spec/std/big/big_decimal_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,36 +337,50 @@ describe BigDecimal do
bd2 = BigDecimal.new(-123, 5)
bd3 = BigDecimal.new(123, 0)
bd4 = BigDecimal.new(-123, 0)
bd5 = "-123.000".to_big_d
bd6 = "-1.1".to_big_d

bd1.to_i.should eq 0
bd2.to_i.should eq 0
bd3.to_i.should eq 123
bd4.to_i.should eq -123
bd5.to_i.should eq -123
bd6.to_i.should eq -1

bd1.to_u.should eq 0
bd2.to_u.should eq 0
bd3.to_u.should eq 123
bd4.to_u.should eq 123
bd5.to_u.should eq 123
Copy link
Copy Markdown
Contributor Author

@Sija Sija Feb 12, 2020

Choose a reason for hiding this comment

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

@bcardiff btw, why do Big*#to_u methods for values < 0 are returning them as absolute instead of raising?

Copy link
Copy Markdown
Member

@straight-shoota straight-shoota Feb 12, 2020

Choose a reason for hiding this comment

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

That seems unexpected...

Copy link
Copy Markdown
Member

@bcardiff bcardiff Feb 12, 2020

Choose a reason for hiding this comment

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

I think is the effect of time, when Int#to_u used to be more like a cast, and BigDecimal#to_big_u simple uses an .abs as an approximation. Nobody submit any opinions regarding that in the original PR #4876 .

It should raise OverflowError for negative inputs.

Copy link
Copy Markdown
Contributor Author

@Sija Sija Feb 12, 2020

Choose a reason for hiding this comment

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

Was thinking the same, although I'd leave it for another PR.

Copy link
Copy Markdown
Member

@straight-shoota straight-shoota Feb 13, 2020

Choose a reason for hiding this comment

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

For further consideration: All these specs don't really test what you would expect. Equality between different number types already works without explicit conversation. So these specs should also validate the return type.

Copy link
Copy Markdown
Contributor Author

@Sija Sija Feb 13, 2020

Choose a reason for hiding this comment

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

@straight-shoota Could we leave it for another PR? I don't think we should do much more here.

Copy link
Copy Markdown
Member

@straight-shoota straight-shoota Feb 13, 2020

Choose a reason for hiding this comment

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

Absolutely

Copy link
Copy Markdown
Contributor Author

@Sija Sija Feb 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

btw, BigInt#to_u / BigFloat#to_u has the same issue:

require "big"

-123.to_big_i.to_u     # => 123
-123.123.to_big_f.to_u # => 123

bd6.to_u.should eq 1

bd1.to_f.should eq 0.00123
bd2.to_f.should eq -0.00123
bd3.to_f.should eq 123.0
bd4.to_f.should eq -123.0
bd5.to_f.should eq -123.0
bd6.to_f.should eq -1.1

bd1.to_i!.should eq 0
bd2.to_i!.should eq 0
bd3.to_i!.should eq 123
bd4.to_i!.should eq -123
bd5.to_i!.should eq -123
bd6.to_i!.should eq -1

bd1.to_u!.should eq 0
bd2.to_u!.should eq 0
bd3.to_u!.should eq 123
bd4.to_u!.should eq 123
bd5.to_u!.should eq 123
bd6.to_u!.should eq 1

bd1.to_f!.should eq 0.00123
bd2.to_f!.should eq -0.00123
bd3.to_f!.should eq 123.0
bd4.to_f!.should eq -123.0
bd5.to_f!.should eq -123.0
bd6.to_f!.should eq -1.1
end

it "hashes" do
Expand Down Expand Up @@ -435,6 +449,9 @@ describe BigDecimal do

it { -2.01.to_big_d.ceil.should eq(-2) }
it { -2.91.to_big_d.ceil.should eq(-2) }

it { "-123.000".to_big_d.ceil.value.should eq(-123) }
it { "-1.1".to_big_d.ceil.value.should eq(-1) }
end

describe "#floor" do
Expand All @@ -445,6 +462,9 @@ describe BigDecimal do
it { 2.11.to_big_d.floor.should eq(2) }
it { 2.91.to_big_d.floor.should eq(2) }
it { -2.91.to_big_d.floor.should eq(-3) }

it { "-123.000".to_big_d.floor.value.should eq(-123) }
it { "-1.1".to_big_d.floor.value.should eq(-2) }
end

describe "#trunc" do
Expand Down
12 changes: 5 additions & 7 deletions src/big/big_decimal.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,8 @@ struct BigDecimal < Number
in_scale(new_scale.scale)
end

private def in_scale(new_scale : UInt64) : BigDecimal
# :nodoc:
def in_scale(new_scale : UInt64) : BigDecimal
Comment thread
Sija marked this conversation as resolved.
if @value == 0
BigDecimal.new(0.to_big_i, new_scale)
elsif @scale > new_scale
Expand Down Expand Up @@ -291,7 +292,8 @@ struct BigDecimal < Number
def ceil : BigDecimal
mask = power_ten_to(@scale)
diff = (mask - @value % mask) % mask
(self + BigDecimal.new(diff, @scale))
value = self + BigDecimal.new(diff, @scale)
value.in_scale(0)
end

def floor : BigDecimal
Expand Down Expand Up @@ -331,11 +333,7 @@ struct BigDecimal < Number

# Converts to `BigInt`. Truncates anything on the right side of the decimal point.
def to_big_i
if self >= 0
self.floor.value
else
self.ceil.value
end
trunc.value
end

# Converts to `BigFloat`.
Expand Down