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

Floating point conversion tests #402

Merged
merged 2 commits into from
Jun 22, 2021
Merged

Floating point conversion tests #402

merged 2 commits into from
Jun 22, 2021

Conversation

la-wu
Copy link
Contributor

@la-wu la-wu commented Jun 17, 2021

This PR adds floating point tests for floatDec and doubleDec. It expects implementations to match base show functionality wrt rounding and (non) shortest-path.

These are based on the original C tests with modifications to match show.

- largely pulled from ryu unit tests
- avoid introducing new functions to test
- we get reasonable exposure to rounding and shortest path even with the
  generic dispatch on exponent
Copy link
Contributor

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

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

Awesome!

@Bodigrim Bodigrim requested a review from sjakobi June 17, 2021 19:24
@sjakobi
Copy link
Member

sjakobi commented Jun 18, 2021

It expects implementations to match base show functionality wrt rounding and (non) shortest-path.

How about a property test that compares the floatDec and doubleDec output to show?

BTW, where does the requirement to match show come from? It sounds like a reasonable idea, but is there an explicit promise that we'll do that or was there a discussion or something like that?

@la-wu
Copy link
Contributor Author

la-wu commented Jun 18, 2021

How about a property test that compares the floatDec and doubleDec output to show?

Do you mean e.g

, testBuilderConstr "floatDec" dec_list floatDec
This test tends to not cover enough cases to exposes differences reliably but is a good statement of expectation.

BTW, where does the requirement to match show come from? It sounds like a reasonable idea, but is there an explicit promise that we'll do that or was there a discussion or something like that?

There was some discussion in the original C PR #222 (comment) and mostly the idea was that it would be 'quite unexpected to diverge from show'.

@Bodigrim
Copy link
Contributor

BTW, where does the requirement to match show come from?

Diverging from it means a breaking change, and of a nasty nature. I think it's desitable to keep show and both ByteString and Text builders in line with each other.

Ideally I'd love to see show fixed in base, but it's likely to take ages, so the shortest path to get ryu merged is to mimic all infelicities.

@sjakobi
Copy link
Member

sjakobi commented Jun 22, 2021

How about a property test that compares the floatDec and doubleDec output to show?

Do you mean e.g

, testBuilderConstr "floatDec" dec_list floatDec

Thanks for making me aware (again) of this test! :)

BTW, where does the requirement to match show come from?

Diverging from it means a breaking change, and of a nasty nature. I think it's desitable to keep show and both ByteString and Text builders in line with each other.

Agreed. It might make sense to explicitly say that floatDec and doubleDec match the show output in their Haddocks. But this PR probably isn't the right place for this.

@Bodigrim Bodigrim merged commit 8a0d222 into haskell:master Jun 22, 2021
@Bodigrim
Copy link
Contributor

Thanks @Lumaere!

@Bodigrim Bodigrim added this to the 0.11.2.0 milestone Jun 22, 2021
Bodigrim pushed a commit to Bodigrim/bytestring that referenced this pull request Jul 1, 2021
* Add floating conversion tests

- largely pulled from ryu unit tests

* Use floatDec and doubleDec in tests

- avoid introducing new functions to test
- we get reasonable exposure to rounding and shortest path even with the
  generic dispatch on exponent
noughtmare pushed a commit to noughtmare/bytestring that referenced this pull request Dec 12, 2021
* Add floating conversion tests

- largely pulled from ryu unit tests

* Use floatDec and doubleDec in tests

- avoid introducing new functions to test
- we get reasonable exposure to rounding and shortest path even with the
  generic dispatch on exponent
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