From 66016cefa2ade1885b19acd258dd00d16a91a04a Mon Sep 17 00:00:00 2001 From: Gabriel Scherer Date: Thu, 10 Feb 2022 22:27:31 +0100 Subject: [PATCH] lib/write: more regular usage protocol for ?buf parameters 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 931050073020064a2182e659fd8d09f8a7e8240f 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.) --- lib/write.ml | 11 ++++++----- lib/write.mli | 15 +++------------ 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/lib/write.ml b/lib/write.ml index 0e2090c9..f098c602 100644 --- a/lib/write.ml +++ b/lib/write.ml @@ -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 @@ -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 diff --git a/lib/write.mli b/lib/write.mli index 5b77accd..364da593 100644 --- a/lib/write.mli +++ b/lib/write.mli @@ -23,13 +23,9 @@ 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 -> @@ -37,13 +33,8 @@ val to_output : ?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 ->