search

c42f / tinyformat

Minimal, type safe printf replacement library for C++
531 stars 75 forks source link

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

Closed willwray closed 5 years ago

willwray commented 5 years ago

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

willwray commented 5 years ago

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 commented 5 years ago

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

c42f commented 5 years ago

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

c42f commented 5 years ago

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".

willwray commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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).

c42f commented 5 years ago

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

c42f commented 5 years ago

Ok tests have passed finally.

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

nigels-com commented 5 years ago

Looks good to me.

c42f commented 5 years ago

Merged!

willwray commented 5 years ago

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 commented 5 years ago

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