On Sun, 2011-02-20, stefan2_at_apache.org wrote:
> Merge all changes (r1068695 - r1072516) from the
> integrate-stream-api-extensions.
>
> These patches add svn_stream_skip(), svn_stream_buffered()
> and svn_stream_supports_mark().
A belated review.
> Modified:
> subversion/trunk/ (props changed)
> subversion/trunk/subversion/include/svn_io.h
> subversion/trunk/subversion/libsvn_subr/stream.c
> subversion/trunk/subversion/libsvn_subr/subst.c
> subversion/trunk/subversion/tests/libsvn_subr/stream-test.c
[...]
> Modified: subversion/trunk/subversion/include/svn_io.h
> +/** Skip data handler function for a generic stream. @see svn_stream_t.
> + * @since New in 1.7.
> + */
> +typedef svn_error_t *(*svn_skip_fn_t)(void *baton,
> + apr_size_t *count);
> +/** Buffer test handler function for a generic stream. @see svn_stream_t
> + * and svn_stream_buffered().
> + *
> + * @since New in 1.7.
> + */
> +typedef svn_boolean_t (*svn_io_buffered_fn_t)(void *baton);
> +/** Set @a stream's skip function to @a skip_fn
> + *
> + * @since New in 1.7
> + */
> +void
> +svn_stream_set_skip(svn_stream_t *stream,
> + svn_skip_fn_t skip_fn);
> +/** Set @a stream's buffer test function to @a buffered_fn
> + *
> + * @since New in 1.7.
> + */
> +void
> +svn_stream_set_buffered(svn_stream_t *stream,
> + svn_io_buffered_fn_t buffered_fn);
> +/**
> + * Skip COUNT bytes from a generic STREAM. If the stream is exhausted
> + * before COUNT bytes have been read, an error will be returned and
> + * COUNT will be changed to the actual number of bytes skipped.
> + *
> + * NOTE. No assumption can be made on the semantics of this function
> + * other than that the stream read pointer will be advanced by *count
> + * bytes. Depending on the capabilities of the underlying stream
> + * implementation, this may for instance be translated into a sequence
> + * of reads or a simple seek operation.
> + */
> +svn_error_t *
> +svn_stream_skip(svn_stream_t *stream,
> + apr_size_t *count);
> +/** Returns @c TRUE if the generic @a stream supports svn_stream_mark().
> + *
> + * @see svn_stream_mark()
> + * @since New in 1.7.
> + */
> +svn_boolean_t
> +svn_stream_supports_mark(svn_stream_t *stream);
Would a better name be svn_stream_supports_mark_seek()?
I do wonder whether we need this. I guess the idea is it's supposed to
be faster than calling svn_stream_mark() and checking for the error
SVN_ERR_STREAM_SEEK_NOT_SUPPORTED.
> +/** Return whether this generic @a stream uses internal buffering.
> + * This may be used to work around subtle differences between buffered
> + * an non-buffered APR files.
> + *
> + * @since New in 1.7.
> + */
> +svn_boolean_t
> +svn_stream_buffered(svn_stream_t *stream);
I renamed the symbols from ...buffered... to ...is_buffered... in
r1130118, for clarity.
Does "is buffered" mean "this layer of the stream implements its own
buffering layer"? Does it mean "this stream knows that there is a RAM
buffer at some layer of the stream implementation, so some reads from
this stream can be served without I/O latency"? Should a layered
stream, for example a checksumming stream, delegate to what its
underlying stream says? The semantics seem insufficiently clear for a
general-purpose stream.
The "is buffered" API has one caller, stream_readline(), which says
if (stream supports mark && is buffered && ...) then
{
/* We can efficiently read chunks speculatively and
reposition the stream pointer to the end of the
line once we found that. */
readline_chunky();
}
else
{
readline_bytewise();
}
But I don't understand that. I understand why we'd need to use a
byte-by-byte implementation if the stream doesn't support mark and seek,
of course, but why would we need to do so if the stream claims not to be
buffered?
The relevant question right now is: is there a clear benefit for this
being public in 1.7? I think not, so I propose to make the "is
buffered" API private by renaming the relevant symbols to
"svn_stream__*". Or move them into private/svn_io_private.h, as Stefan
just suggested. Questions of semantics can then wait.
- Julian
Received on 2011-06-01 16:08:41 CEST