I (Julian Foad) wrote:
> Just a couple of comments, from a quick read of the patch.
>
> +/** Flushes all available internal buffers in a generic @a stream. If
> + * @sync is non-zero, it causes any buffered data to be written to the
> + * underlying device (for example, to the disk for a file stream).
>
> I'm not sure about having the 'sync' flag. What does that mean for a
> non-disk stream? Does the API really need to support two different
> ways of flushing a generic stream? I feel probably not. If we need to
> support two different ways of flushing a disk stream, could we achieve
> that in a better way? Perhaps the code that opens the disk stream
> should specify this option when opening the disk stream, rather than a
> generic writer specifying this option when it calls 'flush'?
Closing a stream always implies first flushing its own buffers. This
proposed svn_stream_flush() API supports flushing a generic stream's
buffers at any (and multiple) points in time. That's potentially
useful, but the present use case only requires us to sync a disk
file's data to disk when closing it. That's a slightly different
thing.
I note that in the MSDN documentation that Evgeny linked to, the plain
'Stream' class has a plain 'Flush' method, while the 'FileStream'
class has (in addition) a Flush(bool) method that supports a 'sync to
disk' option.
Another small point. Ivan mentioned at the beginning of this thread:
> 3. It's unclear how FLUSH_TO_DISK flag should interact with DISOWN=TRUE.
The current patch chooses one of the two possible behaviours for
svn_stream_disown() -- it chooses to forward all flush() method calls
to the 'disowned' stream, and only blocks the close() method. Nothing
wrong with that; I'm just noting that one behaviour was chosen.
- Julian
Received on 2015-05-26 18:01:07 CEST