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