[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: rev 2803 - trunk/subversion/include trunk/subversion/libsvn_wc trunk/subversion/libsvn_client trunk/subversion/tests/clients/cmdline

From: Philip Martin <philip_at_codematters.co.uk>
Date: 2002-07-30 22:40:49 CEST

Karl Fogel <kfogel@newton.ch.collab.net> writes:

> > +/* Return an access baton in ADM_ACCESS associated with PATH. PATH must be
> > + a directory that is locked and part of the set containing the ASSOCIATED
> > + access baton.
>
> Minor consistency issue: sometimes you write FOO when documenting a
> return-by-reference parameter, and sometimes you say *FOO (for
> example, as with "ACCESS_BATON" versus "*LOCKED" above). I believe
> our convention -- though not universally followed -- has generally
> been to use *FOO.

In the case above it's a pointer-to-pointer, so I could make it

 "Return a pointer to a new access baton in *ADM_ACCESS"

Lots of functions take a parameter "svn_wc_adm_access_t *adm_access"
and I've generally written things like

 "Ensure ADM_ACCESS has a write lock".

Do you think that should be changed as well? svn_wc_adm_access_t is
never passed by value.

Do people prefer "working copy context" to "access baton"?

>
> > +/* ### Should this definition go into lock.c? At present it is visible so
> > + ### that users can access the path member, we could provide an access
> > + ### function. There is one place that directly access the lock_exists
> > + ### member as well. */
>
> Hmmm, yeah. +1 on the opaque type, fwiw.

I like the idea of the type being opaque to libsvn_wc as well.

>
> > --- trunk/subversion/libsvn_wc/lock.c (original)
> > +++ trunk/subversion/libsvn_wc/lock.c Tue Jul 30 13:46:26 2002
> > @@ -23,38 +23,365 @@
> >
> > #include "wc.h"
> > #include "adm_files.h"
> > -
> > +#include "svn_pools.h"
> >
> >
> > -svn_error_t *
> > -svn_wc_lock (const char *path, int wait_for, apr_pool_t *pool)
> > +static svn_error_t *
> > +svn_wc__do_adm_close (svn_wc_adm_access_t *adm_access,
> > + svn_boolean_t abort);
> > +
>
> I think the `abort' should be `preserve_lock', right?

Yes.

>
> (The name `abort' is causing a "shadows global symbol" warning during
> compilation right now.)
>
> Also, no need for the `svn_wc__' prefix on file-static functions. But
> they *do* need doc strings :-).

OK.

>
> Btw, we haven't generally done advance prototyping like this, except
> where absolutely necessary. Any objection to just moving things
> around so you don't need the extra prototype?

As I remember, the use of pool cleanup handlers means that at least
one function needs a forward declaration. So it's absolutely
necessary.

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jul 30 22:41:39 2002

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.