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: don't show NaN sign info in Float.toString #1459

Merged
merged 1 commit into from Aug 12, 2022

Conversation

digama0
Copy link
Collaborator

@digama0 digama0 commented Aug 11, 2022

This is another issue identified in the Zulip thread: Because the NaN sign bit and payload bits are architecture-dependent and not specified by IEEE 754, the simplest way to prevent this from affecting Lean code is to treat Float as a quotient type with respect to the various NaN values. Because the actual type is opaque to lean, it is possible to do this "in our hearts" with no code changes. The only exception to this is Float.toString, which becomes a non-function under this model as it distinguishes equal values.

This is still not really observable without native_decide because the kernel doesn't know how any of these functions compute, but we should still try to keep native_decide sound where possible, so the fix implemented here is to make Float.toString print all NaNs the same, as the string "nan". (Some languages like to write it NaN; bikeshed away.)

@digama0 digama0 mentioned this pull request Aug 11, 2022
2 tasks
Copy link
Member

@Kha Kha left a comment

Choose a reason for hiding this comment

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

I have a slight preference for NaN (same as in Rust)

@digama0 digama0 force-pushed the nan_to_string branch 2 times, most recently from 215bca8 to 4997299 Compare August 11, 2022 21:08
@leodemoura leodemoura merged commit 94f85ae into leanprover:master Aug 12, 2022
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.

None yet

3 participants