On 09.02.2011 02:07, Blair Zajac wrote:
> On 2/8/11 4:01 PM, stefan2_at_apache.org wrote:
>> Author: stefan2
>> Date: Wed Feb 9 00:01:04 2011
>> New Revision: 1068695
>>
>> URL: http://svn.apache.org/viewvc?rev=1068695&view=rev
>> Log:
>> Introduce svn_stream_skip() and svn_stream_buffered() stream API
>> functions
>> and implement them for all stream types.
>
> Hi Stefan,
>
> I read through the public headers and it wasn't clear to me what skip
> did. I assumed that the function takes the number of bytes to skip
> and seeks.
>
> typedef svn_error_t *(*svn_skip_fn_t)(void *baton,
> apr_size_t *count);
>
> But in looking at the implementation, it actually consumes all the
> bytes. Maybe it should be renamed to consume, because skip, to me,
> sounds like a cheap seek, instead of a potentially more expensive read.
>
> I suggest adding some more documentation here and/or renaming skip.
> Maybe consume?
"consume" would be false as well since it is supposed to
use seek() or something similarly efficient when supported
by the respective stream. "advance" might be an alternative
but that would be more suitable for iterators / marks.
Therefore, I stick with "skip" for the time being and added
a detailed docstring to the function declaration.
>
>> +
>> +/* Skip data from any stream by reading and simply discarding it. */
>> +static svn_error_t *
>> +skip_default_handler(void *baton, apr_size_t *count, svn_read_fn_t
>> read_fn)
>> +{
>> + apr_size_t total_bytes_read = 0;
>> + apr_size_t bytes_read;
>> + char buffer[4096];
>> + svn_error_t *err = SVN_NO_ERROR;
>> +
>> + while ((total_bytes_read< *count)&& !err)
>> + {
>> + bytes_read = sizeof(buffer)< *count ? sizeof(buffer) : *count;
>
> Since *count isn't being decremented, then you could read past the
> *count bytes if *count isn't evenly divisible by 4096.
Yikes!
The problem is that I used this function for files and strings
only. Both provide specialized implementations.
>
> Sounds like a unit test for skipping is needed.
Definitely. Will provide tests tomorrow or so.
>
>> static svn_error_t *
>> +skip_handler_apr(void *baton, apr_size_t *count)
>> +{
>> + struct baton_apr *btn = baton;
>> + apr_off_t offset = *count;
>> +
>> + SVN_ERR(svn_io_file_seek(btn->file, SEEK_CUR,&offset, btn->pool));
>
> APR_CUR
>
>> + *count = offset;
>
> skip_default_handler() returns the number of bytes actually read while
> skip_handler_apr() returns the current offset. If you were already N
> bytes into the stream, then the two functions don't return the same
> conceptual value.
Fixed.
>
>> +static svn_boolean_t
>> +buffered_handler_apr(void *baton)
>> +{
>> + struct baton_apr *btn = baton;
>> + return apr_file_buffer_size_get(btn->file) != 0;
>
> We have systems with APR 1.2 and it doesn't provide
> apr_file_buffer_size_get().
Is there anything in the 1.3 headers that would indicate
in which version a specific function got introduced?
>
>
>> Modified:
>> subversion/branches/integrate-stream-api-extensions/subversion/libsvn_subr/subst.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/branches/integrate-stream-api-extensions/subversion/libsvn_subr/subst.c?rev=1068695&r1=1068694&r2=1068695&view=diff
>> ==============================================================================
>>
>> ---
>> subversion/branches/integrate-stream-api-extensions/subversion/libsvn_subr/subst.c
>> (original)
>> +++
>> subversion/branches/integrate-stream-api-extensions/subversion/libsvn_subr/subst.c
>> Wed Feb 9 00:01:04 2011
>> @@ -1249,6 +1249,27 @@ translated_stream_read(void *baton,
>> return SVN_NO_ERROR;
>> }
>>
>> +/* Implements svn_skip_fn_t. */
>> +static svn_error_t *
>> +translated_stream_skip(void *baton,
>> + apr_size_t *count)
>> +{
>> + apr_size_t total_bytes_read = 0;
>> + apr_size_t bytes_read;
>> + char buffer[SVN__STREAM_CHUNK_SIZE];
>> + svn_error_t *err = SVN_NO_ERROR;
>> +
>> + while ((total_bytes_read< *count)&& !err)
>> + {
>> + bytes_read = sizeof(buffer)< *count ? sizeof(buffer) : *count;
>
> Same issue here with skipping.
Thanks again for the in-depth review.
-- Stefan^2.
Received on 2011-02-10 02:02:45 CET