Skip to content

Commit

Permalink
lib/write: more regular usage protocol for ?buf parameters
Browse files Browse the repository at this point in the history
The handling of `?buf` parameters today makes little sense:

- `to_string` and all `stream_*` functions clear the buffer before
  using it (so: they guarantee that they work in the same way no
  matter what the input state of the buffer is, and they make no
  guarantees on its output state)

- but `to_channel` and `to_output` make weird claims about it in the
  documentation, that don't make sense to me, and appear to *not* clear
  the buffer and in fact include, in their output, the content of the
  buffer as it is passed.

I believe that the reason for this difference comes from the previous
codebase using Biniou, that was replaced by Buffer in
  9310500

The change left the usage of ?buf in `to_channel` and `to_output`
rather incoherent (and I think broke the documentation somewhat), due
to different usage properties of Biniou_outbuf, who "owns" the
underlying output channel and implements its own buffering, and
Buffer, which is not related to an output channel: the previous API
may have made sense for Binio, but it does not anymore.

What's not completely clear to me is whether some code in the wild
could rely on the previous, nonsensical behaviour. This may happen if
the code was written with Biniou in the past, and converted to use
Buffer in the same systematic (and slightly wrong) way as Yojson
itself.

(Note: there are two independent questions: (1) whether the buffer is
cleared on entry, and (2) whether the buffer is cleared on
exit. Currently for `to_channel` and `to_output` neither is done, and
at least for (2) this seems in direct contradiction with what the
documentation says -- at least with the natural interpretation of "buf
is flushed" as "the content is send to the output channel, and the
buffer is cleared". So the lack of (2) is clearly a bug, and the lack
of (1) might not be.)
  • Loading branch information
gasche committed Feb 10, 2022
1 parent 0fc0bb7 commit 66016ce
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 17 deletions.
11 changes: 6 additions & 5 deletions lib/write.ml
Original file line number Diff line number Diff line change
Expand Up @@ -441,20 +441,21 @@ let to_channel ?buf ?(len=4096) ?std oc x =
let ob =
match buf with
None -> Buffer.create len
| Some ob -> ob
| Some ob -> Buffer.clear ob; ob
in
to_buffer ?std ob x;
Buffer.output_buffer oc ob
Buffer.output_buffer oc ob;
Buffer.clear ob

let to_output ?buf ?(len=4096) ?std out x =
let ob =
match buf with
None -> Buffer.create len
| Some ob -> ob
| Some ob -> Buffer.clear ob; ob
in
to_buffer ?std ob x;
out#output (Buffer.contents ob) 0 (Buffer.length ob);
()
Buffer.clear ob

let to_file ?len ?std ?(newline = true) file x =
let oc = open_out file in
Expand Down Expand Up @@ -487,7 +488,7 @@ let stream_to_channel ?buf ?(len=2096) ?std oc st =
let ob =
match buf with
None -> Buffer.create len
| Some ob -> ob
| Some ob -> Buffer.clear ob; ob
in
stream_to_buffer ?std ob st

Expand Down
15 changes: 3 additions & 12 deletions lib/write.mli
Original file line number Diff line number Diff line change
Expand Up @@ -23,27 +23,18 @@ val to_channel :
?std:bool ->
out_channel -> t -> unit
(** Write a compact JSON value to a channel.
@param buf allows to reuse an existing buffer created with
[Buffer.create] on the same channel.
[buf] is flushed right
before [to_channel] returns but the [out_channel] is
not flushed automatically.
Note: the [out_channel] is not flushed by this function.
See [to_string] for the role of the other optional arguments. *)
See [to_string] for the role of the optional arguments. *)

val to_output :
?buf:Buffer.t ->
?len:int ->
?std:bool ->
< output : string -> int -> int -> int; .. > -> t -> unit
(** Write a compact JSON value to an OO channel.
@param buf allows to reuse an existing buffer created with
[Buffer.add_channel] on the same channel.
[buf] is flushed right
before [to_output] returns but the channel itself is
not flushed automatically.
See [to_string] for the role of the other optional arguments. *)
See [to_string] for the role of the optional arguments. *)

val to_file :
?len:int ->
Expand Down

0 comments on commit 66016ce

Please sign in to comment.