Skip to content

Floating point conversion tests#402

Merged
Bodigrim merged 2 commits intohaskell:masterfrom
0xlwu:float-tests
Jun 22, 2021
Merged

Floating point conversion tests#402
Bodigrim merged 2 commits intohaskell:masterfrom
0xlwu:float-tests

Conversation

@0xlwu
Copy link
Contributor

@0xlwu 0xlwu 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.

0xlwu added 2 commits March 2, 2021 20:52
- 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?

@0xlwu
Copy link
Contributor Author

0xlwu 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.

3 participants