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

Qi double parser IEEE 754 compliance warning #693

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

taam
Copy link

@taam taam commented Aug 2, 2021

This commit adds a warning regarding the issue described in #668. However, I do not know if ...

  • ... a warning is appropriate,
  • ... the placement is ok,
  • ... the statement and wording are actually correct,
  • ... maybe float_ and long_double are affected as well (probably?),
  • ... maybe this applies to real_parser in general.

It's at least a first try. I was also thinking about a longer version, but I didn't want to speculate.

@Kojoley
Copy link
Collaborator

Kojoley commented Aug 2, 2021

Does it really has something to do with IEEE 754 compliance? Unfortunately I do not have a copy of the paper, if the paper does not allow quoting at least provide a paragraph number.

@taam
Copy link
Author

taam commented Aug 2, 2021

I'm do not know, but I was assuming so due to your comments to that issue. I do not have the paper either. Seems I did speculate after all, sorry. That's why I was not eager to get involved into something I have no clue about.

Didn't find any hint searching the web so far, I try to look into other parser implementations if I have time.

@taam
Copy link
Author

taam commented Aug 4, 2021

So far I could not find anything definitive, just hints that there is a single correct conversion, e.g. https://github.com/google/double-conversion/blob/44944accfd5516a094e1e730334764e0a908aaff/double-conversion/strtod.cc very much seems to assume that there is "the correct double".

And maybe even better: https://github.com/fastfloat/fast_float/blame/main/README.md#L32-L34

@djowel
Copy link
Collaborator

djowel commented Aug 5, 2021

If there are tests for conformance, that is probably what we should be looking for and add to our tests.

I offered a way to fix it: promote to a higher precision when available and only when necessary (the if-else branch). It should handle float and double. If there's no way to promote (i.e. T already has the highest precision), a proper fix would have to convert it to a quad [Quadruple precision floating-point]. The promotion from lower precision should be a lower hanging fruit. I'll have a look if I can get some free time.

@taam taam marked this pull request as ready for review August 27, 2022 19:17
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