On 01.06.2011 16:40, Julian Foad wrote:
> 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.
No idea what went on there but this is clearly broken.
Luckily, nobody used the in/out parameter return value.
> 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.
I agree. The "return the true position" was motivated by
the "seek to end-of-stream" use-case that we obviously
don't have anywhere in our code as of now.
Because our streams are often no immediate file streams,
such seeks won't be meaningful in the general case.
Hence, we could not use it without restricting the types
of streams a certain API accepts. That makes it unlikely
that we would use the 'count' out parameter.
>> *count = (apr_size_t)(new_pos - current);
>>
>> return err;
>> }
-- Stefan^2.
Received on 2011-06-02 00:28:41 CEST