On Thu, Jul 08, 2010 at 12:39:22PM +0100, Julian Foad wrote:
> (Oops, I forgot to trim 60 KB of quoted text in my previous reply :-( )
>
>
> Would you mind writing a test. The thing that most needs testing, I
> should think, is interleaving read(), readline(), mark() and seek().
> Here's an idea for a test:
That's a good idea. I'll do that in a separate commit.
> Can you see a convenient way to keep the transformer/filter tests? It
> seems a bit of a pity to delete them.
It is a pity, but I might as well write a new test like you proposed.
> > +/* Invoke the READ_FN function of a stream to scan for an end-of-line
> > + * indicatior, and return it in *EOL.
>
> s/indicatior/indicator
Fixed.
> > + * Use MARK_FN and SEEK_FN to seek in the stream.
>
> Why should it seek in the stream? Ah, because it returns to the
> original position. Let's say so:
>
> Set *EOL to the first end-of-line string found in the stream
> accessed through READ_FN, MARK_FN and SEEK_FN, whose stream baton
> is BATON. Leave the stream read position unchanged. Allocate
> *EOL statically; POOL is a scratch pool.
>
> That also documents the other parameters.
Thanks, I've used this docstring.
> > * Set *EOL to NULL if the stream runs out before an end-of-line indicator
> > * is found. */
> > static svn_error_t *
> > -scan_eol(const char **eol, svn_stream_t *stream, apr_pool_t *pool)
> > +scan_eol(void *baton,
> > + svn_read_fn_t read_fn,
> > + svn_io_mark_fn_t mark_fn,
> > + svn_io_seek_fn_t seek_fn,
> > + const char **eol,
> > + apr_pool_t *pool)
> > {
>
>
> Would it make more sense for stream_create() to initialize the readline
> member to point to the default stream_readline() function, instead of
> writing "if (readline == NULL) X else Y" in two places? That would also
> allow a custom stream impl to set the readline handler to NULL if
> readline doesn't make sense, e.g. on a compress-while-reading stream.
> Not sure if that's better, just asking.
Sure. We can add an error code and make svn_stream_readline() return
an error if readline_fn is NULL. Just like it's done for reset/mark/seek.
> > @@ -1225,11 +1196,14 [...]
> > /**
> > - * Similar to svn_stream_readline(). The line-terminator is detected
> > - * automatically. If @a eol is not NULL, the detected line-terminator
> > - * is returned in @a *eol. If EOF is reached and the stream does not
> > - * end with a newline character, @a *eol will be NULL.
> > + * Similar to svn_stream_readline(). If @a *eol is not NULL, it is used
> > + * as the expected line terminator. If @a eol is NULL, the line-terminator
> > + * is detected automatically. If @a *eol is NULL, the line-terminator is
> > + * detected automatically and is returned in @a *eol.
> > [...]
> > */
> > svn_error_t *
> > svn_stream_readline_detect_eol([...]
>
> Did you mean to change the API here to allow specifying the expected EOL
> string? That seems an unnecessary change, and your impl. doesn't
> support that on non-seekable streams, so it looks unintended:
See my other mail, this change has been reverted.
> > @@ -416,13 +359,35 @@ svn_stream_readline_detect_eol([...]
> > [...]
> > + /* EOL-detection requires mark/seek support. */
> > + if (stream->mark_fn == NULL)
> > + return svn_error_create(SVN_ERR_STREAM_SEEK_NOT_SUPPORTED, NULL, NULL);
> > + if (stream->seek_fn == NULL)
> > + return svn_error_create(SVN_ERR_STREAM_SEEK_NOT_SUPPORTED, NULL, NULL);
>
>
> The empty stream impl looks wrong: who says readline_handler_empty()
> should return APR_EOL_STR as the "detected" EOL string? If that's
> proper behaviour for svn_io_readline_fn_t, it needs to be documented in
> svn_io_readline_fn_t.
Actually, it should set it to NULL to conform to the API. Fixed.
I also forgot to update its parameter list in the last diff I sent :-/
> The forwarding in the several filter streams looks wrong:
>
> > @@ -652,6 +639,7 @@ svn_stream_disown(svn_stream_t *stream, apr_pool_t
> > svn_stream_set_reset(s, reset_handler_disown);
> > svn_stream_set_mark(s, mark_handler_disown);
> > svn_stream_set_seek(s, seek_handler_disown);
> > + svn_stream_set_readline(s, stream_readline);
>
> It should forward to the wrapped stream's readline function, not the
> default readline, shouldn't it? (Several similar places.)
Yes, you're right. Taking that shortcut is wrong because it can
inadvertently override a custom stream method.
> I've only skimmed through it so far, and probably can't do a full review
> before the end of the month. However from what I've seen it looks good
> enough to go ahead and then improve/fix later.
OK, I'll fix up the bits above and commit.
Thanks for review :)
Stefan
Received on 2010-07-08 14:08:59 CEST