(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:
Stream content = "Line one.\nLine two.\n"
# Some points where we will mark and seek:
[[[<1>Line one.
<2>Line <3>two.<4>
<5>]]]
# Sequentially read, mixing read() with readline(), and make marks.
mark(<1>) # before any read or readline
readline() == "Line one."
mark(<2>) # after readline
read(5 bytes) == "Line "
mark(<3>) # after read
read(4 bytes) == "two."
mark(<4>) # after read at EOL
readline() == ""
mark(<5>) # after read after EOL
readline() == EOF
# Jump around with seeks, and check read() and readline().
seek(<1>)
readline() == "Line one."
seek(<5>)
readline() == EOF
seek(<2>)
read(5 bytes) == "Line "
seek(<4>)
readline() == ""
seek(<3>)
readline() == "two."
# And then a test with one stream wrapped in another would be ...
# useful, though harder to write.
Can you see a convenient way to keep the transformer/filter tests? It
seems a bit of a pity to delete them.
> +/* Invoke the READ_FN function of a stream to scan for an end-of-line
> + * indicatior, and return it in *EOL.
s/indicatior/indicator
> + * 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.
> * 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.
> @@ -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:
> @@ -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.
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.)
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.
- Julian
I (Julian Foad) wrote:
> Hi Stefan. I've just started looking at this. Overall impression is it
> looks like you've done it very well - thank you!
>
> Could you possibly get the readline_detect_eol() API changes out of this
> patch, and make them in a separate patch either before or after this?
> Or if that change belongs in this patch, please explain why in the log
> msg.
>
> Thanks.
> - Julian
>
>
> On Wed, 2010-07-07 at 20:39 +0200, Stefan Sperling wrote:
[...]
Received on 2010-07-08 13:41:07 CEST