On Thu, Aug 27, 2009 at 06:51:28PM +0200, Daniel Näslund wrote:
> [[[
> Create a new callback for svn_stream_readline() that can replace, add or
> delete characters from a line.
Comments inline. I'm mainly suggesting changes for the way some things
are worded in the log message and in the docstrings, but that's all
just the way *I* would say it -- there are many ways to say things :)
There is only one more code change which I've suggested below.
Please digest this input to your liking and re-send, I will then
test and commit it.
>
> * subversion/include/svn_io.h
> (svn_io_line_transformer_cb_t): Declare
> (svn_stream_set_line_transformer_callback): Declare
> (svn_stream_readline): Changed doc string.
I'd say something like "Document line transforming in docstring" here.
> * subversion/libsvn_subr/stream.c
You forgot:
(svn_stream_t): Add new field 'transformer_cb'.
> (svn_stream_create): Set transformer_cb to NULL.
"Initialise the new field."
> (svn_stream_set_line_transformer_callback): Sets the new callback.
"New function to set the line transformer callback on a stream."
> * subversion/tests/libsvn_subr/stream-tests.c
> (line_transformer): A callback of the new function. Swaps the order of
> the supplied line.
"An implementation of svn_io_line_transformer_cb_t, reverses the
supplied line."
> (test_stream_line_transformer): tests the line_transformer callback.
"New test testing line transformations on a stream".
> (test_stream_line_filter_and_transformer): tests both line_transformer
> and line_filter callbacks.
"New test testing line transformations as well as line filtering on a stream".
> ]]]
> Index: subversion/libsvn_subr/stream.c
> ===================================================================
> --- subversion/libsvn_subr/stream.c (revision 38964)
> +++ subversion/libsvn_subr/stream.c (arbetskopia)
> @@ -51,6 +51,7 @@
> svn_close_fn_t close_fn;
> svn_io_reset_fn_t reset_fn;
> svn_io_line_filter_cb_t line_filter_cb;
> + svn_io_line_transformer_cb_t line_transformer_cb;
> };
>
>
> @@ -69,6 +70,7 @@
> stream->reset_fn = NULL;
> stream->close_fn = NULL;
> stream->line_filter_cb = NULL;
> + stream->line_transformer_cb = NULL;
> return stream;
> }
>
> @@ -112,6 +114,14 @@
> stream->line_filter_cb = line_filter_cb;
> }
>
> +void
> +svn_stream_set_line_transformer_callback(svn_stream_t *stream,
> + svn_io_line_transformer_cb_t
> + line_transformer_cb)
> +{
> + stream->line_transformer_cb = line_transformer_cb;
> +}
> +
> svn_error_t *
> svn_stream_read(svn_stream_t *stream, char *buffer, apr_size_t *len)
> {
> @@ -207,6 +217,25 @@
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *
> +line_transformer(svn_stream_t *stream, svn_stringbuf_t **buf,
> + const char *line, apr_pool_t *pool)
> +{
> + apr_pool_t *scratch_pool;
> +
> + if (! stream->line_transformer_cb)
> + {
Please check stream->line_transformer_cb != NULL outside of
line_transformer(). The code flow is then easier to understand
in svn_stream_readline() itself. And you don't need to create
the stringbuf inside here.
> + *buf = svn_stringbuf_create(line, pool);
> + return SVN_NO_ERROR;
> + }
> +
> + scratch_pool = svn_pool_create(pool);
> + SVN_ERR(stream->line_transformer_cb(buf, line, pool, scratch_pool));
> + svn_pool_destroy(scratch_pool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> svn_error_t *
> svn_stream_readline(svn_stream_t *stream,
> svn_stringbuf_t **stringbuf,
> @@ -244,10 +273,12 @@
> *eof = TRUE;
>
> SVN_ERR(line_filter(stream, &filtered, str->data, iterpool));
> +
> if (filtered)
> *stringbuf = svn_stringbuf_create_ensure(0, pool);
> - else
> - *stringbuf = svn_stringbuf_dup(str, pool);
> + else
> + SVN_ERR(line_transformer(stream, stringbuf, str->data,
> + iterpool));
So here, something like:
if (filtered)
*stringbuf = svn_stringbuf_create_ensure(0, pool);
else if (stream->line_transformer_cb)
SVN_ERR(line_transformer(stream, stringbuf, str->data, iterpool));
else
*stringbuf = svn_stringbuf_dup(str, pool);
and drop the non-transformed stringbuf creating logic inside of
line_transformer().
>
> return SVN_NO_ERROR;
> }
> @@ -268,6 +299,8 @@
> while (filtered);
> *stringbuf = svn_stringbuf_dup(str, pool);
>
> + SVN_ERR(line_transformer(stream, stringbuf, str->data, iterpool));
> +
> svn_pool_destroy(iterpool);
> return SVN_NO_ERROR;
> }
> Index: subversion/tests/libsvn_subr/stream-test.c
> ===================================================================
> --- subversion/tests/libsvn_subr/stream-test.c (revision 38964)
> +++ subversion/tests/libsvn_subr/stream-test.c (arbetskopia)
> @@ -345,6 +345,105 @@
> return SVN_NO_ERROR;
> }
>
> +/* An implementation of svn_io_line_transformer_cb_t */
> +static svn_error_t *
> +line_transformer(svn_stringbuf_t **buf, const char *line,
> + apr_pool_t *result_pool, apr_pool_t *scratch_pool)
> +{
> + int i, len = strlen(line);
> + char *temp = apr_palloc(scratch_pool, len + 1 );
> +
> + for (i = 0; i < len; i++)
> + {
> + temp[i] = line[len - 1 - i];
> + }
> +
> + temp[len] = '\0';
> +
> + *buf = svn_stringbuf_create(temp, result_pool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +test_stream_line_transformer(apr_pool_t *pool)
> +{
> + static const char *lines[4] = {"gamma", "",
> + "iota", "!"};
> +
> + static const char *inv_lines[4] = {"ammag", "",
> + "atoi", "!"};
> + svn_string_t *string;
> + svn_stream_t *stream;
> + svn_stringbuf_t *line;
> + svn_boolean_t eof;
> +
> + string = svn_string_createf(pool, "%s\n%s\n%s\n%s", lines[0], lines[1],
> + lines[2], lines[3]);
> +
> + stream = svn_stream_from_string(string, pool);
> +
> + svn_stream_set_line_transformer_callback(stream, line_transformer);
> +
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(strcmp(line->data, inv_lines[0]) == 0);
> +
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(strcmp(line->data, inv_lines[1]) == 0);
> +
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(strcmp(line->data, inv_lines[2]) == 0);
> +
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(strcmp(line->data, inv_lines[3]) == 0);
> +
> + /* We should have reached eof and the stringbuf should be emtpy. */
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(eof && svn_stringbuf_isempty(line));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +test_stream_line_filter_and_transformer(apr_pool_t *pool)
> +{
> + static const char *lines[4] = {"!gamma", "",
> + "iota", "!"};
> +
> + static const char *inv_lines[4] = {"ammag", "",
> + "atoi", "!"};
> + svn_string_t *string;
> + svn_stream_t *stream;
> + svn_stringbuf_t *line;
> + svn_boolean_t eof;
> +
> + string = svn_string_createf(pool, "%s\n%s\n%s\n%s", lines[0], lines[1],
> + lines[2], lines[3]);
> +
> + stream = svn_stream_from_string(string, pool);
> +
> + svn_stream_set_line_filter_callback(stream, line_filter);
> +
> + svn_stream_set_line_transformer_callback(stream, line_transformer);
> +
> + /* Line one should be filtered. */
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(strcmp(line->data, inv_lines[1]) == 0);
> +
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(strcmp(line->data, inv_lines[2]) == 0);
> +
> + /* The last line should also be filtered, and the resulting
> + * stringbuf should be empty. */
> + svn_stream_readline(stream, &line, "\n", &eof, pool);
> + SVN_ERR_ASSERT(eof && svn_stringbuf_isempty(line));
> +
> + return SVN_NO_ERROR;
> +
> +}
> +
> +
> +
>
> /* The test table. */
>
> @@ -359,5 +458,9 @@
> "test streams reading from range of file"),
> SVN_TEST_PASS2(test_stream_line_filter,
> "test stream line filtering"),
> + SVN_TEST_PASS2(test_stream_line_transformer,
> + "test stream line transforming"),
> + SVN_TEST_PASS2(test_stream_line_filter_and_transformer,
> + "test stream line filtering and transforming"),
> SVN_TEST_NULL
> };
> Index: subversion/include/svn_io.h
> ===================================================================
> --- subversion/include/svn_io.h (revision 38964)
> +++ subversion/include/svn_io.h (arbetskopia)
> @@ -705,6 +705,20 @@
> const char *line,
> apr_pool_t *scratch_pool);
>
> +/** Character-filtering callback function for a generic
> + * stream.
"A callback function, invoked by svn_stream_readline(), which can perform
arbitrary transformations on the line before it is passed back to the
caller of svn_stream_readline()."
> + * Returns a transformed @a buf allocated in @a result_pool.
"Returns a transformed stringbuf in @a buf, allocated in @a result_pool."
> + * This function gets called on the lines that was not filtered by the
> + * line-filtering callback function.
"This callback gets invoked only on lines which were not filtered by the
line-filtering callback function."
> + *
> + * @see svn_stream_t and svn_stream_readline().
"@see svn_stream_t, svn_io_line_filter_cb_t, and svn_stream_readline()."
> + *
> + * @since New in 1.7. */
> +typedef svn_error_t *(*svn_io_line_transformer_cb_t)(svn_stringbuf_t **buf,
> + const char *line,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool);
> +
> /** Create a generic stream. @see svn_stream_t. */
> svn_stream_t *
> svn_stream_create(void *baton,
> @@ -742,6 +756,14 @@
> svn_stream_set_line_filter_callback(svn_stream_t *stream,
> svn_io_line_filter_cb_t line_filter_cb);
>
> +/** Set @a streams's line-transforming callback function to @a
> + * line_transformer_cb
"@a line_transformer_cb" (both on the same line).
> + * @since New in 1.7. */
> +void
> +svn_stream_set_line_transformer_callback(svn_stream_t *stream,
> + svn_io_line_transformer_cb_t
> + line_transformer_cb);
> +
> /** Create a stream that is empty for reading and infinite for writing. */
> svn_stream_t *
> svn_stream_empty(apr_pool_t *pool);
> @@ -1030,6 +1052,13 @@
> * if they pass the filtering decision of the callback function.
> * If end-of-file is encountered while reading the line and the line
> * is filtered, @a *stringbuf will be empty.
> + *
> + * If a line-transformer callback function was set on the stream using
> + * svn_stream_set_line_transformer_callback(), lines will be returned
> + * transformed in a way determined by the callback.
I'd put a comma after "transformed" to make this sentence easier to
parse.
> + *
> + * Note that the line-transformer callback gets called after the line-filter
> + * callback.
", not before."
> */
> svn_error_t *
> svn_stream_readline(svn_stream_t *stream,
Thanks, looks very good!
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2387976
Received on 2009-08-27 20:31:00 CEST