-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add type restrictions to big #15689
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
Add type restrictions to big #15689
Changes from all commits
de3506e
e29fa4b
13c92e4
1631f89
76e065b
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 |
|---|---|---|
|
|
@@ -81,7 +81,7 @@ struct BigFloat < Float | |
| LibGMP.mpf_get_default_prec | ||
| end | ||
|
|
||
| def self.default_precision=(prec : Int) | ||
| def self.default_precision=(prec : Int) : Nil | ||
|
Collaborator
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. This is worrysome: a setter method should always return its argument.
Contributor
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. There is the older and abandoned #10083 for this
Contributor
Author
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. My own 2-cents: it should be a linting rule rather than a language rule, that the setter return what is handed to it. Seems there are other opinions on what setter methods should return, too (like matching the
Member
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. Yes, we'd want this to be different. But this change is fine for this PR.
Contributor
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. You want it to return what was passed in so that chained assignment is handled. Ruby, for example, ignores returning a value from setters for this reason. PRECISION = BigFloat.default_precision = 5
Member
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. |
||
| LibGMP.mpf_set_default_prec(prec.to_u64) | ||
| end | ||
|
|
||
|
|
@@ -105,11 +105,11 @@ struct BigFloat < Float | |
| LibGMP.mpf_cmp_z(self, other) | ||
| end | ||
|
|
||
| def <=>(other : Float::Primitive) | ||
| def <=>(other : Float::Primitive) : Int32? | ||
|
Contributor
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. All the
Contributor
Author
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. Agreed, and the
Contributor
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.
The contract in general is to return
Contributor
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. This |
||
| LibGMP.mpf_cmp_d(self, other) unless other.nan? | ||
| end | ||
|
|
||
| def <=>(other : Int) | ||
| def <=>(other : Int) : Int32 | ||
| Int.primitive_si_ui_check(other) do |si, ui, big_i| | ||
| { | ||
| si: LibGMP.mpf_cmp_si(self, {{ si }}), | ||
|
|
@@ -369,7 +369,7 @@ struct BigFloat < Float | |
| LibGMP.mpf_get_ui(self).to_u64! | ||
| end | ||
|
|
||
| def to_unsafe | ||
| def to_unsafe : Pointer(LibGMP::MPF) | ||
| mpf | ||
| end | ||
|
|
||
|
|
@@ -473,7 +473,7 @@ struct BigFloat < Float | |
| format_impl(io, is_negative, integer, decimals, separator, delimiter, decimal_places, group, only_significant) | ||
| end | ||
|
|
||
| def clone | ||
| def clone : BigFloat | ||
| self | ||
| end | ||
|
|
||
|
|
@@ -545,7 +545,7 @@ struct Number | |
| end | ||
|
|
||
| struct Int | ||
| def <=>(other : BigFloat) | ||
| def <=>(other : BigFloat) : Int32 | ||
| -(other <=> self) | ||
| end | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,23 +46,23 @@ struct BigRational < Number | |
| # Creates an exact representation of float as rational. | ||
| # | ||
| # Raises `ArgumentError` if *num* is not finite. | ||
| def self.new(num : Float::Primitive) | ||
| def self.new(num : Float::Primitive) : self | ||
| raise ArgumentError.new "Can only construct from a finite number" unless num.finite? | ||
| new { |mpq| LibGMP.mpq_set_d(mpq, num) } | ||
| end | ||
|
|
||
| # Creates an exact representation of float as rational. | ||
| def self.new(num : BigFloat) | ||
| def self.new(num : BigFloat) : self | ||
| new { |mpq| LibGMP.mpq_set_f(mpq, num) } | ||
| end | ||
|
|
||
| # Creates a `BigRational` from the given *num*. | ||
| def self.new(num : BigRational) | ||
| def self.new(num : BigRational) : self | ||
| num | ||
| end | ||
|
|
||
| # :ditto: | ||
| def self.new(num : BigDecimal) | ||
| def self.new(num : BigDecimal) : self | ||
| num.to_big_r | ||
| end | ||
|
|
||
|
|
@@ -90,19 +90,19 @@ struct BigRational < Number | |
| BigInt.new(@mpq._mp_den) | ||
| end | ||
|
|
||
| def <=>(other : BigRational) | ||
| def <=>(other : BigRational) : Int32 | ||
| LibGMP.mpq_cmp(mpq, other) | ||
| end | ||
|
|
||
| def <=>(other : Float::Primitive) | ||
| def <=>(other : Float::Primitive) : Int32? | ||
| self <=> BigRational.new(other) unless other.nan? | ||
| end | ||
|
|
||
| def <=>(other : BigFloat) | ||
| def <=>(other : BigFloat) : Int32 | ||
| self <=> other.to_big_r | ||
| end | ||
|
|
||
| def <=>(other : Int) | ||
| def <=>(other : Int) : Int32 | ||
| Int.primitive_si_ui_check(other) do |si, ui, big_i| | ||
| { | ||
| si: LibGMP.mpq_cmp_si(self, {{ si }}, 1), | ||
|
|
@@ -112,7 +112,7 @@ struct BigRational < Number | |
| end | ||
| end | ||
|
|
||
| def <=>(other : BigInt) | ||
| def <=>(other : BigInt) : Int32 | ||
| LibGMP.mpq_cmp_z(self, other) | ||
| end | ||
|
|
||
|
|
@@ -371,15 +371,15 @@ struct BigRational < Number | |
| denominator.format(io, separator, delimiter, decimal_places, group: group, only_significant: only_significant) | ||
| end | ||
|
|
||
| def clone | ||
| def clone : BigRational | ||
| self | ||
| end | ||
|
|
||
| private def mpq | ||
|
Contributor
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. Why are some of these functions not typed at all? Should be
Contributor
Author
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. The I realize my PR description doesn't show that with the single line command 😬 |
||
| pointerof(@mpq) | ||
| end | ||
|
|
||
| def to_unsafe | ||
| def to_unsafe : Pointer(LibGMP::MPQ) | ||
| mpq | ||
| end | ||
|
|
||
|
|
@@ -406,7 +406,7 @@ struct Int | |
| BigRational.new(self, 1) | ||
| end | ||
|
|
||
| def <=>(other : BigRational) | ||
| def <=>(other : BigRational) : Int32 | ||
| -(other <=> self) | ||
| end | ||
|
|
||
|
|
@@ -467,7 +467,7 @@ end | |
|
|
||
| # :nodoc: | ||
| struct Crystal::Hasher | ||
| def self.reduce_num(value : BigRational) | ||
| def self.reduce_num(value : BigRational) : UInt64 | ||
| inverse = BigInt.new do |mpz| | ||
| if LibGMP.invert(mpz, value.denominator, HASH_MODULUS_INT_P) == 0 | ||
| # inverse doesn't exist, i.e. denominator is a multiple of HASH_MODULUS | ||
|
|
||
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.
This comes from way before this PR, but I would have expected
precisionto be aBigIntegerinBigDecimal🤔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.
FWIW, bigdecimal-rs, which this implementation is based on, does the same