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

legacy format "%.10s" changed behaviour #6534

Closed
vicuna opened this issue Sep 2, 2014 · 5 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link

commented Sep 2, 2014

Original bug ID: 6534
Reporter: Nick Chapman
Assigned to: @gasche
Status: closed (set by @xavierleroy on 2016-12-07T10:34:51Z)
Resolution: fixed
Priority: urgent
Severity: minor
Version: 4.02.0+beta1 / +rc1
Target version: 4.02.1+dev
Category: ~DO NOT USE (was: OCaml general)
Monitored by: @gasche

Bug description

from 4.01 to 4.02

despite the fact that those formats are invalid, we should consider them changing behavior in this release as a bug

Steps to reproduce

OCaml version 4.01.0

Printf.printf "[%.10s]\n" "hello";;

[ hello]

    OCaml version 4.02.1+dev0-2014-08-29

Printf.printf "[%.10s]\n" "hello";;

[hello]

File attachments

@vicuna

This comment has been minimized.

Copy link
Author

commented Sep 2, 2014

Comment author: bvaugon

This kind of format should be written "%10s" instead of "%.10s".

For backward compatibility, I attach a patch that retrieve the legacy behaviour when -strict-formats is omitted. With -strict-formats, this kind of format is still refused.

This patch also fix management of '*' with a negative integer as argument, like in :
printf "<%*d>" (-10) 42

@vicuna

This comment has been minimized.

Copy link
Author

commented Sep 2, 2014

Comment author: @gasche

To clarify: I discussed this example with Nick just before the release, and although everyone agree that this format is incorrect, I proposed that we still classify the regression (change in behavior) itself as a bug, so that we detect those and fix the legacy-support code -- even if it's probably going away in a future version.

@vicuna

This comment has been minimized.

Copy link
Author

commented Sep 4, 2014

Comment author: @damiendoligez

I agree: the regression is a bug, even though this format will be statically refused by 4.03.0.

@vicuna

This comment has been minimized.

Copy link
Author

commented Sep 20, 2014

Comment author: @gasche

Benoît, I'm not really happy with the fix you have in your patch. I feel it is too invasive: it makes the code uglier just to support this legacy format that will be forbidden soon anyway.
I think we should try hard to have the simplest implementation possible, because it is already not that simple.

I merged the part of your patch that did not have this downside: the "negative Arg_lit" part, and some style fixes.
7a7a884
f3095a8

I am considering just keeping this particular issue unsolved, because it is too hard to implement without making the non-legacy path more complex.

There may be a different approach that would be less invasive, but it's hard to tell what could be both simple and type-able -- intuitively the idea is to merge the padding and precision information in your "get_padprec" case, but that's tricky type-checking-wise.
I would try to come up with a solution myself, but for now I'll rather focus on lower-hanging fruits in other bugreports, in preparation for the bugfix release.

@vicuna

This comment has been minimized.

Copy link
Author

commented Sep 21, 2014

Comment author: @gasche

After much playing around with the patch and alternative implementations, I decided that there are many worse ways to do this. I included the patch, with some modifications, in the 4.02 branch, so the change in behavior should now be solved. Thanks Benoît!

@vicuna vicuna closed this Dec 7, 2016

@vicuna vicuna added this to the 4.02.1 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.