Stefan Sperling wrote:
> 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.
Ah. I didn't notice that. I saw it was linked in to the stream in some
way and assumed it was a "method" of the stream, but I was concerned
that it was already sort of outside the scope of a generic stream and
didn't seem to have full support (e.g. of being replaceable) and it's
not clear exactly how it interacts with the plain read() function and I
would assume there should be a writeline() counterpart.
I'm not sure I'd want it to be a standard function of a generic stream
if it isn't already.
> > 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.
Right.
> > 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.
Hmm. Hold the thought for now. That doesn't sound ideal. I'd like to
"mull it over" in my mind for a few days.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388220
Received on 2009-08-28 13:06:04 CEST