On Wed, Nov 08, 2006 at 06:35:18PM -0500, Garrett Rooney wrote:
> >This is a public structure, isn't it? Are there any
> >backward-compatibility considerations we need to worry about?
>
> It's declared in a header file that's private (never installed
> anywhere), and it's only ever shared between svnserve and
> libsvn_ra_svn, which need to be upgraded at the same time anyway, so
> no, I don't think so.
>
You're right, sorry. I was confusing it with svn_ra_svn.h in include/.
> >Those three need comments. It might also be a good idea to explain here
> >vaguely why the ra_svn stream type needs to differ from the general
> >svn_stream_t type. I also wonder whether we really need to duplicate
> >the base stream functionality, or whether we can create a svn_stream_t*
> >from which we can get the underlying ra_svn-specific stuff.
>
> The original reason was that someone (ghudson?) objected to adding the
> required interfaces to svn_stream_t to make this work. If I recall
> correctly the original version of this code did the same stuff via an
> svn_stream_ioctl function or something like that.
>
Yes, I thought I remembered something along those lines. An ioctl
interface is probably a bit sucky, but I honestly can't think of anything
better (svn_stream_t isn't public, so we couldn't even overlay the two
structures in a super-/sub-class style and downcast where necessary).
> >> +static svn_boolean_t pending(apr_pollfd_t *pfd, apr_pool_t *pool)
> >> +static svn_error_t *
> >> +file_read_cb(void *baton, char *buffer, apr_size_t *len)
> >> +static svn_error_t *
> >> +file_write_cb(void *baton, const char *buffer, apr_size_t *len)
> >> +static void
> >> +file_timeout_cb(void *baton, apr_interval_time_t interval)
> >> +static svn_boolean_t
> >> +file_pending_cb(void *baton)
> >> +static svn_error_t *
> >> +sock_read_cb(void *baton, char *buffer, apr_size_t *len)
> >> +static svn_error_t *
> >> +sock_write_cb(void *baton, const char *buffer, apr_size_t *len)
> >> +static void
> >> +sock_timeout_cb(void *baton, apr_interval_time_t interval)
> >> +static svn_boolean_t
> >> +sock_pending_cb(void *baton)
> >
> >While I can guess what these all do, it would be nice if there was some
> >minimal commenting.
>
> Honestly, here I disagree, you're talking about functions where the
> comments would be almost as long as the actual code, and would add no
> meaning. If you look at the code this replaced there was perhaps one
> comment, and that still exists in the new code. The fact that the
> code has migrated into a function does not mean it immediately needs a
> comment saying that it does exactly what you'd expect it to do based
> on the name of the function and the interface it implements.
>
I was really just angling for a minimal
/* Functions to implement the ra_svn_stream... interface for
(file-backed | socket-backed) streams. */
kind of thing. I agree that there's no point in documenting each one in
turn, but a reference to the interface would be nice. (And something to
explain what pending() does, since it's not part of either of those
groups, it looks like).
Regards,
Malcolm
- application/pgp-signature attachment: stored
Received on Thu Nov 9 00:52:26 2006