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

Enforce precision in printf %F #2262

Merged
merged 1 commit into from Mar 9, 2019

Conversation

@proux01
Copy link
Contributor

commented Feb 23, 2019

Printf.printf "%.4F" or Printf.printf "%.17F" were all behaving as Printf.printf "%F", ignoring the precision.

Copy link
Member

left a comment

I suspect the patch is correct (thanks for preserving backward-compatibility by keeping a different default for F), but I would like to see testsuite coverage for it. Would you add tests in testsuite/tests/lib-printf/tprintf.ml?

stdlib/camlinternalFormat.ml Outdated Show resolved Hide resolved
@proux01 proux01 force-pushed the proux01:printf-F branch from 7088cf8 to 2d72685 Feb 23, 2019
@proux01

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

Thanks for the fast review!

Would you add tests in testsuite/tests/lib-printf/tprintf.ml?

Indeed, just had to uncomment/fix the existing ones.

@proux01 proux01 force-pushed the proux01:printf-F branch 3 times, most recently from 00d3fff to 439d550 Feb 23, 2019
stdlib/camlinternalFormat.ml Outdated Show resolved Hide resolved
Copy link
Member

left a comment

Approved in principle -- the feature is sensible and the implementation looks fine (I made a minor change suggestion, but the code is already nice). This is good to merge when everyone is happy and the CI passes.

You should add a Changes entry in the "Working version" section, in a new ### Standard library: category (there is no stdlib entry yet).

@proux01 proux01 force-pushed the proux01:printf-F branch 2 times, most recently from 2935bbf to 8653bc7 Feb 24, 2019
@proux01

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

I took the opportunity to also add + and space flags to %F. As the float_conv then became a fully expanded cartesian product {,+,space} x {f,e,E,,g,G,F,h,H}, I encoded it as a pair. I thus had to regenerate boot/ocamlc (using make bootstrap). I hope I haven't done anything wrong. Otherwise, I'm ok with it and CI is green.

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

This needs another round of review, and I don't have time to do it myself before March 2nd. If no one else does it, please feel free to ping here sometime next week in case I forgot about it.

@gasche gasche self-requested a review Feb 26, 2019
@gasche gasche dismissed their stale review Feb 26, 2019

previous version of the PR

@proux01

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

@gasche ping (just because you asked, we're definitely not in a hurry)

@gasche
gasche approved these changes Mar 9, 2019
Copy link
Member

left a comment

I reviewed it just now, thanks! It is actually a very nice simplification, makes you wonder whether we could eventually use this style for int_conv as well -- but it doesn't support the full cartesian product of options for now, even if we take C/# out.

I approve the PR (again), but see a minor clarification suggestion on the char_of_fconv function. If think that it's not worth it, I would be ok for merging as-is.

stdlib/camlinternalFormat.ml Outdated Show resolved Hide resolved
@proux01 proux01 force-pushed the proux01:printf-F branch from 8653bc7 to ddf4540 Mar 9, 2019
@proux01

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

Thanks for the review!

makes you wonder whether we could eventually use this style for int_conv as well -- but it doesn't support the full cartesian product of options for now, even if we take C/# out.

Flags + and space don't make any sense for unsigned integers (u for instance) so I'm not very optimistic here.

@proux01 proux01 force-pushed the proux01:printf-F branch from ddf4540 to ed74b5b Mar 9, 2019
@gasche gasche merged commit aaea44c into ocaml:trunk Mar 9, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.