On Fri, Aug 28, 2009 at 10:29:25AM +0100, Julian Foad wrote:
> Hi Daniel and others.
>
> We have recently added a "line filter" callback and a "line transformer"
> callback to svn_stream_t's "readline" function. I can see they're
> useful, but now I'm wondering whether they are the right approach to
> extending a stream's functionality.
I did the line filter originally, and the transformer was modeled after
it. Back when I added the line filter I saw svn_stream_readline() as a
separate utility function for streams, since the stream has no function
pointer to it. The way I saw it was that it's just a helper. But see below.
> The idea of a generic stream is that the API is generic and a particular
> stream implementation provides any desired functionality. The "readline"
> function is a concession to how often streams are used for plain text,
> but in most respects special data handling is done by creating a wrapper
> stream which does the special handling during reads and/or writes and
> passes all the other calls straight through to the wrapped stream.
>
> My stream of thoughts since the new callbacks were added has gone like
> this.
>
> First: All callbacks should take a baton. Patch attached. (Shall I just
> commit this?)
Possibly, though right now, the readline function is not part of the
stream itself, so passing the stream baton into it feels a bit weird.
> Second: These two callbacks do a very similar job: optionally modify the
> line, and optionally delete it entirely. I don't think it makes much
> sense for them to be separate. We should just have one callback that can
> modify or delete the line as it wishes.
> Third: If svn_stream_readline() can pass each line through a generic
> filter callback, that's pretty similar to just having replaced the
> "readline" function with a different "readline" function, in the same
> way that svn_stream_set_read() replaces the stream's basic "read"
> function with a different "read" function. Should we not use the same
> approach here, and provide svn_stream_set_readline() instead of
> svn_stream_set_line_filter_callback()?
> So, ultimately, I'm thinking that this filtering/modifying should be
> done by a wrapper stream.
>
> Does this all make sense?
Yes, it might make sense now to promote the readline function from a helper
to a proper stream method. However with the current mechanism we don't
duplicate the "read a line" logic anywhere, and I'd like to keep it that
way. So if we make streams define their own readline() method, we should
do it in a way that avoids duplicating code, by factoring out the
"read a line" functionality into a static helper function which can be
used by svn_stream_readline() implementations.
The standard stream's readline method would just call this helper.
Then, we implement two new types of streams:
- A stream to read original text from a patch file, overriding the
readline method.
- A stream to read modified text from a patch file, overriding the
readline method.
A stream to read latest text from a file already exists :)
And then we use those in the unit tests and patch code instead of
the filter/transformer callbacks.
I guess this can be made to work nicely.
So +1 on the approach, someone just has to do it.
(/me looks at dannas for a blink of a second)
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388213
Received on 2009-08-28 12:24:17 CEST