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

std.format: Add formatting integers with %e, %f, %g and %a. #8015

Merged
merged 3 commits into from
May 7, 2021

Conversation

berni44
Copy link
Contributor

@berni44 berni44 commented Apr 26, 2021

Inspired by issue 16078 I added formatting with %e etc. for integers. I tried to reach an identical result to first casting and then formatting, while preserving all digits:

writefln!"%.10e"(1234567890);
writefln!"%.10e"(cast(float) 1234567890);

yields:

1.2345678900e+09
1.2345679360e+09

As can be seen: With the cast, the last four digits are lost while they are preserved without.

I decided to deviate from the floating point behavior in two places:

  • Default precision isn't anymore 6, but just as large as necessary to keep all digits. The reason for this change is, that the trailing fractional digits of floating point values normally don't contain any information and thus can be cut away. This is not true for integrals and so I think it's better to preserve this information as a default.
  • With %a, the integral digit can be larger than 1. %a tries to resemble the mantissa and therefore the integral digit is always 0 or 1 and the fractional digits match with the fractional digits of the mantissa. Integral values don't have a mantissa nor have they fractional digits. So it's more natural to build them from right to left instead of left to right which is natural for floating point values. But that leads to larger integral digits. (And as a side effect, the digits of %a and %x are the same which is also preferable.)

The first commit adds a rounding tool. I plan to use this to simplify the functions in std.format.internal.floats. Therefore I put it into a separate function.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @berni44! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
21847 enhancement std.format: %e, %g and %a should be supported for integers too

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8015"

Copy link
Collaborator

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the addition, but I am unable to review code. cc @ibuclaw @andralex @rainers

@berni44
Copy link
Contributor Author

berni44 commented Apr 27, 2021

I agree with the addition, but I am unable to review code. cc @ibuclaw @andralex @rainers

I'd like to add @thewilsonator to this list. He did a lot of similar reviews in the past. :-)

@RazvanN7 RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label May 4, 2021
@n8sh
Copy link
Member

n8sh commented May 4, 2021

Default precision isn't anymore 6, but just as large as necessary to keep all digits. The reason for this change is, that the trailing fractional digits of floating point values normally don't contain any information and thus can be cut away. This is not true for integrals and so I think it's better to preserve this information as a default.

I would say that the appropriate mental model here is that requesting to format an integral type as if it were floating point performs implicit conversion to floating point (as occurs for functions or variables), so the behavior should match.

@berni44
Copy link
Contributor Author

berni44 commented May 4, 2021

I would say that the appropriate mental model here is that requesting to format an integral type as if it were floating point performs implicit conversion to floating point (as occurs for functions or variables), so the behavior should match.

I agree on this mental model. I used it for everything (aside from defaults). I even wrote an external test comparing the output to the output of a cast, to make sure I didn't miss anything.

So, when I read your post, I wondered, why I disagree in case of defaults. Meanwhile I think, it's because in my opinion the defaults of floats are not chosen well and they are just kept for backward compatibility. (See issue 17715 where someone complains about these defaults - it could have been me. Or An introduction to floating point numbers, something I wrote some weeks ago - I don't remember why - but it shows, how the current defaults cause problems.)

All in all, I think, that my gut feeling just tells me, that it would be a good thing to avoid transfering these problems from floats to integers.

@n8sh
Copy link
Member

n8sh commented May 4, 2021

in my opinion the defaults of floats are not chosen well

I'm not a fan of them either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Severity:Enhancement std.format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants