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

Add support for %a,%A format specifiers - hex float output #17 #53

Closed
wants to merge 10 commits into from

Conversation

willwray
Copy link
Contributor

@willwray willwray commented Apr 4, 2019

Add tests for formatted output, width modifier and precision (ignored).
(Only tested on recent GCC so please test with the CI targets.)

Add tests for formatted output, width modifier and precision (NA).
Fix two typos.
@willwray
Copy link
Contributor Author

willwray commented Apr 4, 2019

So, all the Windows appveyor CI platforms are failing on tests
and the console output doesn't give a clue on the cause.
I can run the tests successfully on Visual Studio 2019 preview.
I'll attempt with older MSVC toolchains.

@c42f
Copy link
Owner

c42f commented Apr 5, 2019

Thanks this looks great. We'll need to dig into the CI issues but other than that LGTM.

@c42f
Copy link
Owner

c42f commented Apr 5, 2019

I added the -VV flag to ctest appveyor so hopefully we'll be able to see what's going wrong now.

@c42f
Copy link
Owner

c42f commented Apr 5, 2019

Ok there you go: the tests are failing due to older MSVC generating extra zero padding. Is there a way to work around that? That also makes me wonder whether we need a test for long fractional part with %a as the spec for handling of default precision for %a seems possibly "interesting".

Stream output of hexfloat should ignore stream precision.
This change cooks the tests so that they output exactly the default 6 hexits
@willwray
Copy link
Contributor Author

willwray commented Apr 5, 2019

According to STL (himself) this sounds like an MSVC bug
so I filed a report with Developer Community
hexfloat-stream-output-does-not-ignore-precision
I've prepared a workaround commit.

@willwray
Copy link
Contributor Author

willwray commented Apr 5, 2019

Still fails on VS 12 where precision(0) means 0.
In later versions precision(0) means default precision which is 6.
In fact, precision should be ignored.
I'll just remove that test.

willwray added 2 commits April 6, 2019 14:49
The format string specifies precision as "%.13a" so that the
test will pass on MSVC.
@willwray
Copy link
Contributor Author

willwray commented Apr 6, 2019

That also makes me wonder whether we need a test for long fractional part

Good idea.
And it can be done without special-casing for MSVC...
The length limit for hexfloat double-precision fractional part is exactly 13 hexits.
So, by specifying full precision in the format string as "%.13a" an output requiring
full precision will work on conforming implementations which ignore the precision
and output the minimum number of hexits required - in this case the full 13.
Or, on non-conforming MSVC, the specified precision is honored with the same result.

13 hexits, at 4 bits per hexit, corresponds exactly to the 52-bit significand of an
IEEE binary64 'double', e.g.:

    format("%.13a", 0.1)    ->   0x1.999999999999ap-4

I was wondering whether we should disallow precision spec in the %a format string.
I vote that we leave it as is, and document it as an incompatibility, because that allows
this workaround for MSVC to output full precision which would appear to be the only
way to guarantee lossless roundtrip conversion in that case.

@c42f
Copy link
Owner

c42f commented Apr 16, 2019

Sorry for the slow response, I've been away on holiday.

I was wondering whether we should disallow precision spec in the %a format string.

My guiding principle: what do otherwise conforming printf implementations do? If they error, so should we.

Regarding non-conforming MSVC versions, I think we should simply force precision to 13 digits for MSVC to ensure the primary(?) goal of hexfloat: guaranteed round-trippable conversions. If the resulting strings contain a few extra zeros, so be it (the files will be a bit bigger, but nothing will break).

Chris Foster added 2 commits April 17, 2019 14:48
Force precision to 13 in this case to guarentee roundtripping of double
precision hexfloat formatted numbers with MSVC.
@c42f
Copy link
Owner

c42f commented Apr 17, 2019

Ok, I've pushed some changes which implement my suggested workaround for MSVC. I hope this will do the trick now.

@c42f
Copy link
Owner

c42f commented Apr 17, 2019

Ok tests have passed finally.

I think this could be merged if the current version satisfies your use case?

@nigels-com
Copy link
Contributor

Looks good to me.

@c42f
Copy link
Owner

c42f commented May 1, 2019

Merged!

@willwray
Copy link
Contributor Author

willwray commented May 1, 2019

I'd missed some of action here so thanks for going ahead.

The workaround for MSVC, conditional compile setting full precision, is the best that can be done for now.
Once the bug is fixed then the condition can add a "< bugfix_version".
I checked fmt lib on MSVC - the version I pulled (latest on vcpkg) also outputs full precision
(I think that this is fixed in a recent commit, but using charconv or their own conv code).

@c42f
Copy link
Owner

c42f commented May 1, 2019

I hope what I ended up with works for you.

I've always resisted writing my own conversion code because that seems to be a massive rabbit hole which I don't want to go down. Besides, other excellent libraries have the "let's write a lot of code so we can just do printf safely and as fast as possible" niche covered. The flip side of trying to be minimal is having to put up with some bugs, as here...

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