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

Improved printing for floats #563

Merged
merged 2 commits into from Oct 25, 2017

Conversation

Projects
None yet
3 participants
@scurest
Contributor

scurest commented Oct 24, 2017

All floats I've tried now print correctly.

See #375 (comment) for the first commit. The further changes are

  • -0.0 now prints correctly, thanks @tiehuis
  • you actually need 9 digits for f32s (I think 8 would be enough if you used the next digit to round, but if there's a lot of 9s at the end you'd need to carry and it'd be a pain); for f64s, I just use the whole buffer Errol outputs
  • fixed some more small typos in our Errol code
  • Errol has a bug, possibly? See marcandrysco/Errol#8. I had to make the change I mentioned in that issue to get the tests to succeed.

For the last bullet, maybe we want to wait for an upstream confirmation and patch.

Tests

The tests I made are here (see the usage file at the bottom). All the tests I've run have passed. In particular the f32 tests are intended to show by exhaustion that we're correct for all f32s.

  • f32 I iterate over all positive u32s, bitCasting them to f32s, skipping the NaNs and Infs, and printing the rest to stdout in zig. This is piped into a C program where I scanf with %f and check if the nth one I get bitcasts back to n. This should exhaustively test all positive f32s.

    If you want to check all f32s, just change the 0x80000000s to 0xffffffffs in both files. I didn't just because it takes about 2 hours to run already.

  • rand This generates some random numbers with xoroshiro128+, bitcasts to f64, prints them, pipes them into C, scanfs them with %lf and checks if they're the same. The more times you run it, the greater your confidence should be that we're correct :)

@tiehuis

This comment has been minimized.

tiehuis commented on std/fmt/index.zig in 03a0dfb Oct 23, 2017

This will also handle the case where x is -0.0. You can use the signbit function to determine if x is negative zero here.

This comment has been minimized.

Owner

scurest replied Oct 24, 2017

Will do, thanks. I actually didn't know negative zero printed with a minus sign!

More corrections to float printing
Testing suggests all f32s are now printed accurately.
@andrewrk

This comment has been minimized.

Member

andrewrk commented Oct 24, 2017

Excellent work, @scurest.

I'm going to wait for the tests to pass, and then merge.

@scurest

This comment has been minimized.

Contributor

scurest commented Oct 24, 2017

Ah, I forgot to ask: I couldn't get the zig files for the tests to compile in release mode. How do you do it?

@andrewrk

This comment has been minimized.

Member

andrewrk commented Oct 24, 2017

Are you asking how to run a test in release mode?

zig test foo.zig --release-fast
or
zig test foo.zig --release-safe

@scurest

This comment has been minimized.

Contributor

scurest commented Oct 24, 2017

Well, I meant compile say f32-out.zig in release mode.

If I use zig build-exe --release-safe f32-out.zig I get linker errors from lld for __fixunsdfti, __umodti3, __udivti3, etc. The u128 stuff.
If I add --library c I get a binary but that binary only prints three floats before printing "index out of bounds" and segfaulting.

@andrewrk

This comment has been minimized.

Member

andrewrk commented Oct 25, 2017

If I use zig build-exe --release-safe f32-out.zig I get linker errors from lld for __fixunsdfti, __umodti3, __udivti3, etc. The u128 stuff.

Hmm. This is really surprising, I'm looking into this right now. At first glance it seems like LLVM optimization is deleting functions that are marked with linkonce linkage, which according to http://llvm.org/docs/LangRef.html#linkage-types seems like shouldn't happen.

andrewrk added a commit that referenced this pull request Oct 25, 2017

fix missing compiler_rt in release modes
the optimizer was deleting compiler_rt symbols, so I changed
the linkage type from LinkOnce to Weak

also changed LinkOnce to mean linkonce_odr in llvm and
Weak to mean weak_odr in llvm.

See #563
@andrewrk

This comment has been minimized.

Member

andrewrk commented Oct 25, 2017

I fixed the compiler-rt problem in master branch. The other problems you ran into I see the same problem, and it's a bug in zig. I'm looking into it. I'll merge this PR though, because this is better than status quo.

@andrewrk andrewrk merged commit 262b742 into ziglang:master Oct 25, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@scurest scurest deleted the scurest:float-printing branch Oct 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment