[RFC] Commonize further code between Fixed and Normed
#168
Conversation
This includes following breaking changes:
- `Bool` throws the InexactError for inputs other than zero/one.
- The return type of `Integer(::Normed{T})` is now `T`.
- `Integer(::Fixed)` throws the InexactError instead of MethodError.
This includes a breaking change in the return type of `Rational(::Fixed)`.
|
Regarding the changing the return types, users can use some concrete type names instead (e.g. |
Codecov Report
@@ Coverage Diff @@
## master #168 +/- ##
==========================================
+ Coverage 88.12% 88.64% +0.52%
==========================================
Files 5 5
Lines 379 370 -9
==========================================
- Hits 334 328 -6
+ Misses 45 42 -3
Continue to review full report at Codecov.
|
| # constructor-style conversions | ||
| (::Type{X})(x::Real) where {X <: FixedPoint} = _convert(X, x) | ||
|
|
||
| function (::Type{<:FixedPoint})(x::AbstractChar) |
There was a problem hiding this comment.
I wonder if we still need this. Do you fail the ambiguity check if you delete it? (I ask because the supertype of AbstractChar is now Any but IIRC it used to be Integer.)
There was a problem hiding this comment.
I think the rawtype T should not be an AbstractChar type. However, whether to allow conversion from a Char is up to the implementation, regardless of what its supertype is. I left this fool-proof just because I had no reason to remove it. So, Let's get rid of it.
There was a problem hiding this comment.
FYI:
JuliaLang/julia#8816
JuliaLang/julia#26286
It seems that AbstractChar was added long after the supertype of Char had been changed.
There was a problem hiding this comment.
My guess is that this was added purely as a means to avoid an ambiguity. In the old days, ambiguities printed as warnings when you loaded the package, which was really annoying.
There was a problem hiding this comment.
The trigger is certainly the ambiguities. (cf. PR #105)
What I want to say in the comment above is that this may be intentionally left as a fool-proof.
I thought I would get a MethodError, but the conversion is done without errors.
julia> Normed{UInt32,16}('a')
97.0N16f16| end | ||
| function (::Type{Ti})(x::FixedPoint) where {Ti <: Integer} | ||
| isinteger(x) || throw(InexactError(:Integer, typeof(x), x)) | ||
| floor(Ti, x) |
There was a problem hiding this comment.
Would it be better to combine these (compute the output first and then check for equality)? Or does that miss some corner cases?
There was a problem hiding this comment.
FixedPointNumbers doesn't know where or what errors floor(Ti, x) throws. So, it is better to check first for a clear error message.
I don't want to optimize the methods which can throw errors, in principle.
There was a problem hiding this comment.
Of course, when isinteger uses floor, it can be optimized simply. It was obvious to me, so I forgot to write a TODO comment. 😄
|
For now, I will merge this as is. |
These commits fix #154 and commonize:
BoolandIntegerconversionsRationalconversionsThis PR introduces some breaking changes:
Boolthrows the InexactError for the inputs other than zero/one.Integer(::Normed{T})is nowT.Integer(::Fixed)throws the InexactError instead of MethodError.Rational(::FixedPoint{T})is nowRational{T}, exceptFixed{Int8,8},Q0f7,Q0f15and so on.