On Thu, Aug 27, 2009 at 03:08:35PM +0100, Julian Foad wrote:
> 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.
Doc string added.
> 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.
I have changed the callback to return a new stringbuf. You're right,
the flag was just confusing.
> > @@ -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.
Fixed.
> One more thing. You need to define and state whether the transform is
> applied before or after the filter, and implement it accordingly. In
> this v2 implementation, you have a mixture of both: you filter first,
> but then go on to call line_transformer() on the resulting stringbuf
> which may be empty as a result of the filter.
Yeah, that was not intended. Fixed.
>
> It's great that you already included a test for the transformer. It
> would be really good if you could add a test for transforming and
> filtering at the same time, as that seems to be an areas where there is
> a lot of room for confusion and bugs.
>
Added another test that uses both callbacks.
A new log message:
[[[
Create a new callback for svn_stream_readline() that can replace, add or
delete characters from a line.
* 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.
* subversion/libsvn_subr/stream.c
(svn_stream_create): Set transformer_cb to NULL.
(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. Swaps the order of
the supplied line.
(test_stream_line_transformer): tests the line_transformer callback.
(test_stream_line_filter_and_transformer): tests both line_transformer
and line_filter callbacks.
]]]
Thanks Julian and stsp for the feedback!
Mvh
Daniel
Received on 2009-08-27 18:51:50 CEST