-
Notifications
You must be signed in to change notification settings - Fork 33
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
Fix Ambiguities #105
Fix Ambiguities #105
Conversation
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.
Awesome, thanks!
src/fixed.jl
Outdated
@@ -7,6 +7,9 @@ struct Fixed{T <: Signed,f} <: FixedPoint{T, f} | |||
Fixed{T, f}(i::Integer, _) where {T,f} = new{T, f}(i % T) | |||
Fixed{T, f}(x) where {T,f} = convert(Fixed{T,f}, x) | |||
Fixed{T, f}(x::Fixed{T,f}) where {T,f} = x | |||
Fixed{T, f}(x::Char) where {T,f} = throw(ArgumentError("Fixed cannot be constructed from a Char")) | |||
Fixed{T, f}(x::Complex) where {T,f} = Fixed{T, f}(real(x)) |
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.
real
returns the real part even if there is a non-zero complex part. Would it be better to use convert
and get the InexactError
?
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.
Yes, that is a blunder. The reason I ended here is that Complex{S}
, for some reason, didn't match the ambiguous signature. Guess it will have to be Fixed{T, f}(convert(real(eltype(x)), x))
. Will push a fix.
c6ebe4e
to
91b2f53
Compare
Add some tests
Codecov Report
@@ Coverage Diff @@
## master #105 +/- ##
=========================================
+ Coverage 91.44% 91.7% +0.26%
=========================================
Files 4 4
Lines 187 193 +6
=========================================
+ Hits 171 177 +6
Misses 16 16
Continue to review full report at Codecov.
|
I think it should be ready now. I've also added some tests. |
Great, thanks again! |
Fixes #101