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

wrong Printf.printf output for integers with '0' flag #6938

Closed
vicuna opened this issue Jul 24, 2015 · 11 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link

commented Jul 24, 2015

Original bug ID: 6938
Reporter: acascella
Status: closed (set by @xavierleroy on 2017-02-16T14:14:43Z)
Resolution: fixed
Priority: normal
Severity: minor
Platform: x86_64
OS: GNU/Linux
OS Version: 3.13.0-57-generi
Version: 4.02.2
Target version: 4.03.0+dev / +beta1
Fixed in version: 4.03.0+dev / +beta1
Category: standard library
Tags: patch
Monitored by: @gasche

Bug description

Printf.printf doesn't handle correctly the '0' flag for the integer conversion specifiers 'ld' and 'Ld'. This flag should be ignored if a precision is specified (to be consistent with the C standard).

Currently (from version 4.02.0), padding zeros will be added to fill the whole print width instead of padding only to the specified precision.

The behavior is correct for the 'd' conversion specifier (but wasn't working properly in 4.02.0 though).

Steps to reproduce

OCaml version 4.03.0+dev9-2015-07-22:

Printf.printf "%047.27d" 1736201459;;

               000000000000000001736201459- : unit = ()

Printf.printf "%047.27Ld" 1736201459L;;

0000000000000000000000000000000000001736201459- : unit = ()

Printf.printf "%047.27ld" 1736201459l;;

00000000000000000000000000000000000001736201459- : unit = ()

Additional information

The bug was introduced in version 4.02.0 where all the int formats were broken. Partial fix for the 'd' conversion specifier was introduced in 4.02.1 but the bug is still present for the 'Ld' and 'ld' conversion specifiers.

(Everything worked fine in version 4.01.0, see output below).

    Ocaml version 4.01.0

Printf.printf "%047.27Ld" 1736201459L;;

                000000000000000001736201459- : unit = ()

Printf.printf "%047.27ld" 1736201459l;;

                000000000000000001736201459- : unit = ()

Printf.printf "%047.27d" 1736201459;;

                000000000000000001736201459- : unit = ()

    OCaml version 4.02.0

Printf.printf "%047.27d" 1736201459;;

00000000000000000000000000000000000001736201459- : unit = ()

Printf.printf "%047.27Ld" 1736201459L;;

00000000000000000000000000000000000001736201459- : unit = ()

Printf.printf "%047.27ld" 1736201459l;;

00000000000000000000000000000000000001736201459- : unit = ()


    OCaml version 4.02.1

Printf.printf "%047.27d" 1736201459;;

               000000000000000001736201459- : unit = ()

Printf.printf "%047.27Ld" 1736201459L;;

00000000000000000000000000000000000001736201459- : unit = ()

Printf.printf "%047.27ld" 1736201459l;;

00000000000000000000000000000000000001736201459- : unit = ()


    OCaml version 4.03.0+dev9-2015-07-22:

Printf.printf "%047.27d" 1736201459;;

               000000000000000001736201459- : unit = ()

Printf.printf "%047.27Ld" 1736201459L;;

0000000000000000000000000000000000001736201459- : unit = ()

Printf.printf "%047.27ld" 1736201459l;;

00000000000000000000000000000000000001736201459- : unit = ()

File attachments

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 24, 2015

Comment author: @gasche

Thank you for this detailed report.

The 4.02.1 commit that reintroduced this corner behaviour is
dc7e024

I didn't know that the C standard mandates this -- I think Benoît and I wrongly assumed that was a quirk of the existing implementation. We should indeed fix %ld etc. to have the same backward-compatibility behavior, as you suggest.

Note that if what you want is space padding, you can get it by using an actual space padding indication with "% 47" instead of "%047":

Printf.printf "% 47.27ld" 1736201459l;;

                000000000000000001736201459- : unit = ()

Printf.printf "% 47.27Ld" 1736201459L;;

                000000000000000001736201459- : unit = ()

I would agree with the comment in the patch that "%047.27d" should rather be rejected statically, because its behaviour makes little sense.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 24, 2015

Comment author: @dbuenzli

This flag should be ignored if a precision is specified (to be consistent with the C standard).

Just to be sure I understood well, it's not the 0 flag that should be ignored but the [width] if there is a [.precision] ?

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 24, 2015

Comment author: @gasche

No, it's almost the other way around, with a hack thrown in the middle.

For floats, ".27" means to use 27 digits of precision, and "027" means to pad the number zeroes (on the left). Just using "27" (eg. %27f) just means padding with spaces (on the left). For integers and family, "%.27d" is interpreted exactly as "%027d" (pad to the left with zeroes"). But then you can also use "%047.27d", and then the "0" is ignored: it means the same as "%47.27d", that is: pad with 0 upto length 27, then with spaces upto length 47. (Note that "%47027d" could not be used, as it pads with 47027 spaces)

Which means, by the way, that my first message above is slightly wrong (which means I should be just writing my thesis instead of padding format strings): to pad with spaces it suffices to write "%47.27ld", the space is unnecessary. The space means "pad positive numbers with a space, so that they have the same length as negative numbers" (the other option being "+", to pad with a + sign). Oh well.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 24, 2015

Comment author: acascella

@dbuenzli: gasche is right. It is the '0' flag that should be ignored, not the width.

@gasche: actually I do not agree with the comment you point to, because simply ignoring the '0' flag will not only solve this issue, but also remain consistent with C (and thus avoiding future issues that might be introduced with other workarounds).

[edit] I think that "%47027d" is a perfectly valid format: it will print a lot of spaces and then your integer (for a total of 47027 characters).

Incidentally, this behavior was noticed when trying to emulate C's printf with OCaml's and failing to pass some consistency tests: http://trust-in-soft.com/tis-interpreter/

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 24, 2015

Comment author: @gasche

I think warning on "%047.27d" (or at least having a mode that rejects it statically, as we have today) is a good thing, because this format makes strictly less sense than the equivalent "%47.27d", so we should encourage authors to clarify their intent, and either recognize that they intended something else or use the clearer expression. That said I agree that, when we accept this format, the runtime behavior should be consistent (with C, the legacy implementation, etc.).

Incidentally, this behavior was noticed when trying to emulate C's printf with OCaml's and failing to pass some consistency tests: http://trust-in-soft.com/tis-interpreter/

This seems very interesting, do you have a list of other test cases? If you felt motivated to contribute to the testsuite we have for formats (testsuite/tests/lib-{printf,scanf,scanf-2,format}), that could help spot other regressions.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 24, 2015

Comment author: bvaugon

I agree. In fact, this problem seems to have been fixed properly
for "%d" between 4.02.0 and 4.02.1:
-> "%042.27d" is currently rejected when the -strict-formats option
is passed to the compiler
-> "%042.27d" is accepted without -strict-formats and have the same
behaviour as in C, ie. the same as for "%42.27d"

I attach a patch that propagates this fix to "%ld", "%Ld" and "%nd" in
the same way. This patch is for the trunk, but is also applicable
(with some auto patch offsets) on the 4.02.1 release.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 24, 2015

Comment author: @dbuenzli

What about the various other integer printing formats %n|l|L ?

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 24, 2015

Comment author: bvaugon

They are also fixed by the patch.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 25, 2015

Comment author: @gasche

I forgot to say: the patch looks good (of course!) and it's added to my merge list for trunk -- unless someone merges it before.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 25, 2015

Comment author: acascella

This seems very interesting, do you have a list of other test cases? If you felt motivated to contribute to the testsuite we have for formats (testsuite/tests/lib-{printf,scanf,scanf-2,format}), that could help spot other regressions.

I am currently automatically generating test cases. Once I get good coverage I might extract some meaningful cases to contribute to the test suite.

@vicuna

This comment has been minimized.

Copy link
Author

commented Oct 27, 2015

Comment author: @xavierleroy

Fix committed in July 2015 (907305c). Marking this PR as resolved.

@vicuna vicuna closed this Feb 16, 2017

@vicuna vicuna added the stdlib label Mar 14, 2019

@vicuna vicuna added this to the 4.03.0 milestone Mar 14, 2019

@vicuna vicuna added the bug label Mar 20, 2019

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