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

fix_dec, ini_dec change float(x) to Float64(x) #32868

Closed
wants to merge 1 commit into from
Closed

fix_dec, ini_dec change float(x) to Float64(x) #32868

wants to merge 1 commit into from

Conversation

JeffreySarnoff
Copy link
Contributor

The prior definitions allowed stack overflow when the floating point type defined float(x) to return a value that, while Real and AbstractFloat, was not an expected IEEEFloat. These two edits remedy that in the least intrusive yet comprehensive way.

The prior definitions allowed stack overflow when the floating point type defined `float(x)` to return a value that, while `Real` and `AbstractFloat`, was not an expected IEEEFloat.  These two edits remedy that in the least intrusive yet comprehensive way.
@StefanKarpinski
Copy link
Sponsor Member

This feels like it's too specific. What if a type is known to be too large for a Float64? With float it's at least possible to hook into the logic and add an appropriate method giving the appropriate type. With Float64 here that's no longer possible.

@JeffreySarnoff
Copy link
Contributor Author

JeffreySarnoff commented Aug 13, 2019

That is true -- except that using float and giving a floating point type larger than Float64 is was caused the stack to overflow as the result of float was a floating point value larger than Float64 which dispatched ini_dec(::Real) over and over. Do you know what this is intended to do with e.g. Double64 because float(x::Double64) === x as fits the definition of float. Indeed
float(x::T) where {T <: AbstractFloat} = x and ini_dec, fix_dec do not provide workable fallbacks then. [converting to BigFloat and passing those values did not work either -- dunno why, as BigFloats per se seem to be handled]

I am open to a better approach. I guess step 1 is understanding those two functions and why they exist and how they are used -- this is the only source file in which they appear.

@quinnj
Copy link
Member

quinnj commented Aug 13, 2019

Yeah, when you get down to it, you have to see what the underlying algorithms support; over at #32859, I actually changed it from float to Float64, because realistically the Ryu algorithm expects that, Float32, or Float16 explicitly. Could we have a good generic fallback that did a slightly slower thing (using BigInt/BigFloat?)? Probably. But switching this to Float64 better reflects what our current fallbacks need.

@JeffreySarnoff
Copy link
Contributor Author

well said. given the underlying algorithmic implicit type constraints, rather than float(x) [which is the identity operation for any ::AbstractFloat and many ::Real types], the function required is fastfloat(x) --- unless rewriting that part underlying Printf is to be done. fastfloat(x) is x for IEEEFloats and Float64(x) otherwise. (I do not see the advantage in this use to have the result be Float32(x) for [U]Int8 and [U]Int16 values. If introducing this function for export, then that refinement makes sense.)

@JeffreySarnoff
Copy link
Contributor Author

JeffreySarnoff commented Aug 13, 2019

@StefanKarpinski <oops ... no functional air conditioning for days ... nvr mnd>

@quinnj
Copy link
Member

quinnj commented Sep 8, 2020

Made unnecessary by #32859

@quinnj quinnj closed this Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants