On Fri, Aug 28, 2009 at 03:57:32AM +0300, Daniel Shahaf wrote:
> Stefan Sperling wrote on Thu, 27 Aug 2009 at 13:35 -0700:
> > Log:
> > Create a new callback for svn_stream_readline() that can perform
> > arbitrary transformations on a line before it is returned to the
> > caller of svn_stream_readline().
> >
> ...
> > +++ trunk/subversion/libsvn_subr/stream.c Thu Aug 27 13:35:11 2009 (r38973)
> > @@ -207,6 +217,25 @@ line_filter(svn_stream_t *stream, svn_bo
> > return SVN_NO_ERROR;
> > }
> >
> > +/* Run the line transforter callback of STREAM with LINE as input,
> > + * and expect the transformation result to be returned in BUF,
> > + * allocated in POOL. */
> > +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;
> > +
> > + scratch_pool = svn_pool_create(pool);
> > + SVN_ERR(stream->line_transformer_cb(buf, line, pool, scratch_pool));
> > + svn_pool_destroy(scratch_pool);
> > +
>
> So, we create/destroy a pool for *every single line* in the stream?
> (when a transformation callback is set)
>
> Isn't that too expensive? Perhaps this function (line_transformer)
> should simply take two pool arguments like everyone else?
True. But we would have to convert svn_stream_readline() to dual-pool
as well. Creating the scratch pool inside of svn_stream_readline() won't
fix this problem. That's not a small change, but it could be done.
It's definitely a change that warrants a separate patch.
> > Modified: trunk/subversion/tests/libsvn_subr/stream-test.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/tests/libsvn_subr/stream-test.c?pathrev=38973&r1=38972&r2=38973
> > ==============================================================================
> > --- trunk/subversion/tests/libsvn_subr/stream-test.c Thu Aug 27 13:29:01 2009 (r38972)
> > +++ trunk/subversion/tests/libsvn_subr/stream-test.c Thu Aug 27 13:35:11 2009 (r38973)
> > @@ -345,6 +345,105 @@ test_stream_line_filter(apr_pool_t *pool
> > 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)
>
> Reverses a string. The comment could say that?
It probably should. But it's a small helper in a test which won't be
looked at often by many people. How much brainpower did you have to
apply to find out that it was reversing a string? In case you had to
apply a lot, it was probably a good exercise :)
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388200
Received on 2009-08-28 11:52:17 CEST