Hi,
The doc fixes were commited a few days ago and merged onto 1.1. The code
changes will go in shortly. Below are comments where I don't agree/have
questions.
On Mon, 12 Jul 2004 kfogel@collab.net wrote:
> > +static const char *
> > +get_attr (const char **atts, const char *which)
> > +{
> > + for (; atts && *atts; atts += 2)
> > + if (strcmp (*atts, which) == 0)
> > + return atts[1];
> > + return NULL;
> > +}
>
> Hmmm. I feel like we've got similar code to this elsewhere in ra_dav,
> let's see... Yes, in fetch.c, *and* in log.c, the exact same code.
> Before we had it in two places, now we have it in three.
>
> This is getting ridiculous. I think it's time to factor this
> somewhere. Of course, you're not the original sinner, just the most
> recent :-), but would you like to fix this?
>
Actually, it was already factored in svn_xml.h (svn_xml_get_attr_value)...
So I killed these statics.
> > +/* If an error occurs, store it and abort the XML parse. */
> > +#define CHKERR(e) \
> > +do { \
> > + if ((rb->err = (e)) != NULL) \
> > + return NE_XML_ABORT; \
> > +} while(0)
>
> Same factorization comment applies here.
This is a little ugly, since it uses a local variable that's not a macro
argument and that has different types in the different files. But can can
say it takes a second arg that should be a struct with a member err. Can I
just put it in dav_svn.h with the same name, or do we have namespace
considerations for that file?
> > +static svn_error_t *
> > +svn_ra_local__get_file_revs (void *session_baton,
> > + const char *path,
> > + svn_revnum_t start,
> > + svn_revnum_t end,
> > + svn_ra_file_rev_handler_t handler,
> > + void *handler_baton,
> > + apr_pool_t *pool)
>
> Same thing about mentioning where to find the doc string.
If you scroll backwards in that file, you have a comment saying "The RA
plugin routines". Then, none of the plugin functions are commented. I
think we should add docstrings to all or none.
>
> > +static svn_error_t *ra_svn_get_file_revs(void *session_baton, const char *path,
> > + svn_revnum_t start, svn_revnum_t end,
> > + svn_ra_file_rev_handler_t handler,
> > + void *handler_baton, apr_pool_t *pool)
> > +{
>
> Same thing about mentioning what prototype this implements.
>
And nearly the same answer... We should add all or none IMO.
> > +/* Write out a list of property diffs. PROPDIFFS is an array of svn_prop_t
> > + * values. */
> > +static svn_error_t *write_prop_diffs(svn_ra_svn_conn_t *conn,
> > + apr_pool_t *pool,
> > + apr_array_header_t *propdiffs)
>
> Is it really 'svn_prop_t', or is it 'svn_prop_t *'?
>
> > +{
> > + int i;
> > +
> > + for (i = 0; i < propdiffs->nelts; ++i)
> > + {
> > + const svn_prop_t *prop = &APR_ARRAY_IDX(propdiffs, i, svn_prop_t);
> > +
> > + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "c(?s)",
> > + prop->name, prop->value));
> > + }
> > +
> > + return SVN_NO_ERROR;
> > +}
> > +
>
> Wow, so it's really 'svn_prop_t'. Okay. But then why bother to take
> its address? Why not just do
>
> const svn_prop_t prop = APR_ARRAY_IDX(propdiffs, i, svn_prop_t);
> SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "c(?s)",
> prop.name, prop.value));
>
> in that case?
>
I don't see why copying the struct makes the code more readable.
> Comments like the ones above are only to be expected in a change this
> large, please don't take them as my reaction to the change as a whole.
> It's a very solid piece of work -- bravo!
>
And I like these detailed reviews because they maintain code quality.
Thanks for taking the time.
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jul 19 23:55:14 2004