[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] v3. line_transformer callback

From: Daniel Näslund <daniel_at_longitudo.com>
Date: Thu, 27 Aug 2009 18:51:28 +0200

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.

> 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!


Received on 2009-08-27 18:51:50 CEST

This is an archived mail posted to the Subversion Dev mailing list.