Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement floating point conversion with ryu #365
Implement floating point conversion with ryu #365
Changes from 36 commits
ba61414
7d80321
356ed6c
c0a0583
ff2210d
0bb5a01
de1d174
8dd7f16
23d5cfe
755f58f
4635e2b
76b5e2e
b5f7086
648bfae
d172abf
5f3dce5
f1c6275
c2c2c87
f6497c2
60db980
6ec7e2d
a73c84e
3ccfd47
6b3eaaa
abf6e04
f771cd5
b394896
9815597
c0648bb
d87d3ae
5500d59
7d7d7fa
906d6db
046a42b
8fafed4
dde95e2
415ac6f
0474332
9be8170
f67df50
a01cb00
0cc5417
12436a2
d8dac2a
b70918b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 paragraph is a bit hard to understand I think. An example would be very helpful. Please also clarify what "this implementation" refers to. Maybe create a named chunk and refer to it from the relevant places.
Also note that single quotes (
'
) are used for marking identifiers in Haddock. Escaping the quotes might work?! E.g.\'shortest\'
.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.
I added an example / handwavy explanation about the rounding / boundaries and pointed to the paper for details. TBH at this point I am far enough from the writing this code that I would need to redigest the exact semantics.
WRT quotes, thanks for pointing that out. I'm used to normal MD.
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 raises
Pattern match(es) are non-exhaustive
with GHC 9.2.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.
Hm is there a way to convince GHC that the list is non-empty? Looking at the implementation of
roundTo
, the non-throwing return values are(0, _)
and(1, 1:_)
. Before the call todigitsToZero
, we prepend0
to the former case'ssnd
value so AFIACT this is 'correct'.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.
We could also just mimic the case above or add some redundant matches e.g
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.
I'm not saying that your code is wrong, I'm saying that we need to get rid of a warning. Boot packages cannot have warnings. Throwing an
error
for an empty list would be fine, for instance.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.
I agree that this warning needs to be silenced. Maybe this would be a good fit for
Data.List.NonEmpty
?!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.
I added the snippet above. A lot of the code in
GHC.Float
makes the same assumption though so do they just turn the warning off?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 module uses the pragma