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.

Sign up for GitHub

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

Jump to bottom

Stdlib: added Printf.ikbprintf #9364

Merged
merged 1 commit into from Mar 26, 2020
Merged

Stdlib: added Printf.ikbprintf #9364

merged 1 commit into from Mar 26, 2020

Conversation

muskangarg21
Copy link
Contributor

fixes #7110
@Octachron please review. Thanks

@gasche
Copy link
Member

gasche commented Mar 14, 2020 •
edited

The code is exactly the same as ikfprintf, and the type is also the same. I didn't expect this (it is surprising), but I think that it means that ikfprintf is in fact generic enough to be used for "ignoring printers" whatever their accumulator type is (out_channel for fprintf, Buffer.t for bprintf).

This generality was not initially present (in released versions) when the original issue #7110, it was added by #201 at around the same time. (I reviewed this generalizing change but didn't realize at the time that it applied to i{k,}bprintf.)

I think that there are two choices we could make:

  1. have ibprintf and ikbprintf be documented as aliases of ifprintf and ikfprintf (the point is to let people change code by just adding i, even if the other function would have done the job)
  2. not adding new functions, but documenting in the existing i[k]fprintf that it also works for the buffer version

Personally I think that (1) is the nicer option for users. This is an extension of the work currently proposed in this PR, except that:

  • you could/should just do let ikbprintf = ikfprintf, instead of re-expanding to take arguments
  • the documentation should point out that ikfprintf and ikbprintf are general synonyms of each other (but it's fine to keep the documentation you already include, which describes (correctly) the relation between kbprintf and ikbprintf)
  • I think we should also add ibprintf, so that the {i,}k{b,f}printf functions and the {i,}{b,f}printf functions have exactly the same structure

@muskangarg21
Copy link
Contributor Author

@gasche is this what you had in mind ?

@gasche
Copy link
Member

gasche commented Mar 14, 2020

This looks good to me, thanks! (I think it would be nice to also have someone else's opinion on the matter.)

The @SInCE tags should not be "@SInCE 4.10", but "@SInCE 4.11", given that they will only be available in the next release.

@muskangarg21
Copy link
Contributor Author

This looks good to me, thanks! (I think it would be nice to also have someone else's opinion on the matter.)

The @SInCE tags should not be "@SInCE 4.10", but "@SInCE 4.11", given that they will only be available in the next release.

Okay, I ll update that.
@Octachron your views on this ?

@yallop
Copy link
Member

yallop commented Mar 14, 2020

(I think it would be nice to also have someone else's opinion on the matter.)

My preference would be to restrict the type of ikbprintf so that it's the same as the type of kbprintf (and similarly for ibprintf/bprintf). More general types are not always better: there's no advantage in being able to write ibprintf stdout "%d" 3 (almost certainly a mistake), for example, which the current code allows.

@Octachron
Copy link
Member

I tend to agree with @yallop, it is probably better to keep the specific type forikbprintf to make sure that ikbrintf and kbprintf can always be swapped for each other in any context.

I also think it would be nice to add a ibprintf function for the sake of symmetry.

Octachron
Octachron reviewed Mar 19, 2020
View reviewed changes
stdlib/printf.ml Outdated
@@ -22,10 +22,13 @@ let kbprintf k b (Format (fmt, _)) =
make_printf (fun acc -> bufput_acc b acc; k b) End_of_acc fmt
let ikfprintf k oc (Format (fmt, _)) =
make_iprintf k oc fmt
let ikbprintf k bc (Format (fmt, _)) =
make_iprintf k bc fmt
Copy link
Member

Choose a reason for hiding this comment

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

You can keep the equality let ikbprintf = ikfprintf here, even if we do not expose the equality in the interface.

Octachron
Octachron reviewed Mar 20, 2020
View reviewed changes
stdlib/printf.mli Outdated
@@ -147,6 +147,12 @@ val ifprintf : 'b -> ('a, 'b, 'c, unit) format4 -> 'a
@since 3.10.0
*)

val ibprintf : Buffer.t -> ('a, Buffer.t, 'c, unit) format4 -> 'a
Copy link
Member

Choose a reason for hiding this comment

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

It is better to use the format type (and not format4) to get exactly the same type as bprintf.

Octachron
Octachron reviewed Mar 20, 2020
View reviewed changes
stdlib/printf.mli Outdated
@@ -175,6 +181,13 @@ val kbprintf : (Buffer.t -> 'd) -> Buffer.t ->
@since 3.10.0
*)

val ikbprintf : (Buffer.t -> 'd) -> Buffer.t ->
('a, Buffer.t, 'c, 'd) format4 -> 'a
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, we want to restrict 'c to unit to keep the exact same type as kbprintf.

@Octachron Octachron merged commit 4019c33 into ocaml:trunk Mar 26, 2020
@Octachron
Copy link
Member

Merged, thanks for the contribution!

@muskangarg21 muskangarg21 changed the title [WIP] Stdlib: added Printf.ikbprintf Stdlib: added Printf.ikbprintf Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Octachron Octachron Octachron left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing Printf.ikbprintf
4 participants
@muskangarg21 @gasche @yallop @Octachron