I (Julian Foad) wrote:
> 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.
And some more.
> static svn_error_t *
> skip_handler_apr(void *baton, apr_size_t *count)
> {
> struct baton_apr *btn = baton;
> apr_off_t offset = *count;
> apr_off_t new_pos = *count;
> apr_off_t current = 0;
> svn_error_t *err;
>
> /* so far, we have not moved anything */
> *count = 0;
>
> SVN_ERR(svn_io_file_seek(btn->file, APR_CUR, ¤t, btn->pool));
> err = svn_io_file_seek(btn->file, APR_CUR, &new_pos, btn->pool);
>
> /* Irrespective of errors, return the number of bytes we actually moved.
> * If no new position has been returned from seek(), assume that no move
> * happend and keep the *count==0 set earlier.
> */
> if ((offset != new_pos) || (current == 0))
It doesn't make sense to compare 'offset' with 'new_pos', because the
former is a relative position and the latter an absolute position. And
comparing 'current' with 0 doesn't seem relevant: if the first seek
failed, we'd not still be in this function because an error would have
been raised.
If the second seek returns an error you can't trust the value of
'new_pos', unless we're relying on undocumented behaviour. So I think
what you want here is simply:
if (! err)
and remove the doc-string promise that you'll return the current
position in the event of an error. And then the 'count' parameter can
be just an input parameter rather than in/out.
> *count = (apr_size_t)(new_pos - current);
>
> return err;
> }
- Julian
Received on 2011-06-01 16:40:51 CEST