Daniel Näslund wrote:
> [[[
> Creata a new callback for svn_stream_readline() that can replace, add or
> delete characters from a line.
>
> * subversion/include/svn_io.h
> (svn_stream_set_line_transformer_callback): Declare
>
> * subversion/libsvn_subr/stream.c
> (svn_stream_set_line_transformer_callback): Sets the new callback
>
> * subversion/tests/libsvn_subr/stream-tests.c
> (line_transformer): A callback of the new function. Removes leading
> '!' characters. Does not allocate a new line.
> (test_stream_line_transformer): tests the above callback.
> ]]]
Hi Daniel. This looks good in principle.
You need to state the required behaviour of an
svn_io_line_transformer_cb_t callback function in that type's
doc-string. Mention all the parameters and what it must do with them.
For example, one thing that isn't clear is whether such a callback
function must set '*line' to anything at all if it doesn't want to
transform it. Another is whether the resulting '*line' must be allocated
in 'pool' or can alternatively point to the input string or a static
string.
Please give the new function 'line_transformer' a doc string, even
though 'line_filter' doesn't have one. Or, as it is very simple, you
could perhaps remove it and write the necessary test in line.
> svn_error_t *
> svn_stream_readline(svn_stream_t *stream,
> svn_stringbuf_t **stringbuf,
> @@ -217,6 +243,7 @@
> svn_stringbuf_t *str;
> apr_pool_t *iterpool;
> svn_boolean_t filtered;
> + const char *temp;
>
> iterpool = svn_pool_create(pool);
> do
> @@ -249,6 +276,11 @@
> else
> *stringbuf = svn_stringbuf_dup(str, pool);
>
> + SVN_ERR(line_transformer(stream, &filtered, &temp, str->data,
> + iterpool));
> + if (filtered)
> + *stringbuf = svn_stringbuf_create(temp, pool);
> +
> return SVN_NO_ERROR;
> }
>
This block is handling the end-of-stream case. Your code is transforming
'str->data' but the previous lines (not visible in this quote) have just
decided that 'str' might not be the right result because it might have
been filtered out. I think you need '*stringbuf' instead.
Re-using the boolean variable 'filtered' to also indicate whether it was
transformed is a little confusing and error-prone. Please create a
second variable.
> @@ -266,8 +298,14 @@
> SVN_ERR(line_filter(stream, &filtered, str->data, iterpool));
> }
> while (filtered);
> - *stringbuf = svn_stringbuf_dup(str, pool);
>
> + SVN_ERR(line_transformer(stream, &filtered, &temp, str->data,
> + iterpool));
> + if (filtered)
> + *stringbuf = svn_stringbuf_create(temp, pool);
> + else
> + *stringbuf = svn_stringbuf_dup(str, pool);
> +
> svn_pool_destroy(iterpool);
> return SVN_NO_ERROR;
> }
In this block, which is thandling the normal case, it looks like the 'if
(filtered)' test (which means "if transformed") is redundant because
your line_transformer() function does that job.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2387892
Received on 2009-08-27 16:08:59 CEST