You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Peter N. Lundblad" <pe...@famlundblad.se> on 2005/10/17 09:14:03 UTC
r16754
David,
Here is a review of r16754 which was proposed for 1.3 backport. The real
problem with this is that it exports some symbols from the wrong library,
which we might have to live with.
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 16753)
> +++ subversion/include/svn_wc.h (revision 16754)
> @@ -443,6 +443,17 @@
>
>
> /**
> + * Return a duplicate of @a item, allocated in @a pool. No part of the new
> + * item will be shared with @a item.
> + *
> + * @since New in 1.3
Missing period.
> Index: subversion/libsvn_subr/constructors.c
> ===================================================================
> --- subversion/libsvn_subr/constructors.c (revision 16753)
> +++ subversion/libsvn_subr/constructors.c (revision 16754)
> @@ -22,6 +22,7 @@
> #include "svn_types.h"
> #include "svn_props.h"
> #include "svn_string.h"
> +#include "svn_client.h"
>
>
> svn_commit_info_t *
> @@ -67,16 +68,104 @@
> return new_changed_path;
> }
>
> +/**
> + * Reallocate the members of @a prop using @a pool.
> + */
We don't use doxygen comments for internal functions. We use normal comments
and capitalize argument names (see HACKING).
> +static void
> +svn_prop_member_dup (svn_prop_t *prop, apr_pool_t *pool)
This function is static, but has a name that indicates it is
exported. And it should be named ..._members_dup (plural).
> +{
> + if (prop->name)
> + prop->name = apr_pstrdup (pool, prop->name);
> + if (prop->value)
> + prop->value = svn_string_dup (prop->value, pool);
> +}
> +
> svn_prop_t *
> svn_prop_dup (const svn_prop_t *prop, apr_pool_t *pool)
> {
> - svn_prop_t *new_prop = apr_pcalloc (pool, sizeof (*new_prop));
> + svn_prop_t *new_prop = apr_palloc (pool, sizeof (*new_prop));
>
> - if (prop->name)
> - new_prop->name = apr_pstrdup (pool, prop->name);
> - if (prop->value)
> - new_prop->value = svn_string_dup (prop->value, pool);
> + *new_prop = *prop;
>
> + svn_prop_member_dup (new_prop, pool);
> +
> return new_prop;
> }
>
> +/**
> + * Duplicate a @a hash containing (char * -> svn_string_t *) key/value
> + * pairs using @a pool.
> + */
> +static apr_hash_t *
> +svn_string_hash_dup (apr_hash_t *hash, apr_pool_t *pool)
Same here about docstring format and function name.
> +{
> + apr_hash_index_t *hi;
> + const void *key;
> + apr_ssize_t klen;
> + void *val;
> + apr_hash_t *new_hash = apr_hash_make (pool);
> + for (hi = apr_hash_first (pool, hash); hi; hi = apr_hash_next (hi) )
Get rid of that last space before the last paren.
> + {
> + apr_hash_this (hi, &key, &klen, &val);
> + key = apr_pstrdup (pool, key);
> + val = svn_string_dup (val, pool);
> + apr_hash_set (new_hash, key, klen, val);
> + }
> + return new_hash;
> +}
> +
> +svn_client_proplist_item_t *
> +svn_client_proplist_item_dup (const svn_client_proplist_item_t *item,
> + apr_pool_t * pool)
> +{
> + svn_client_proplist_item_t *new_item
> + = apr_pcalloc (pool, sizeof (*new_item));
> +
> + if (item->node_name)
> + new_item->node_name = svn_stringbuf_dup (item->node_name, pool);
> +
> + if (item->prop_hash)
> + new_item->prop_hash = svn_string_hash_dup (item->prop_hash, pool);
> +
> + return new_item;
> +}
> +
This doesn't belong in libsvn_subr.
> +/**
> + * Duplicate an @a array of svn_prop_t items using @a pool.
> + */
> +static apr_array_header_t *
> +svn_prop_array_dup (const apr_array_header_t *array, apr_pool_t *pool)
Name and docstring format.
> +svn_client_commit_item2_t *
> +svn_client_commit_item2_dup (const svn_client_commit_item2_t *item,
> + apr_pool_t *pool)
Wrong lib (as above).
The code in itself looks good.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r16754
Posted by Julian Foad <ju...@btopenworld.com>.
David James wrote:
> On 10/17/05, David James <ja...@gmail.com> wrote:
>>On 10/17/05, Peter N. Lundblad <pe...@famlundblad.se> wrote:
>>>
>>>We could add a utils.c like in libsvn_wc where we can put other small
>>>functions if we need to.
>>
>>Ok, I'll do this. I'll also make the svn_prop_array_dup function
>>public, so that it can be used by svn_client_commit_item2_dup.
>
> Committed in r16744. I also fixed the issues with style and naming
> convention. This change has been nominated for backport to 1.3.x.
For the record, I think that was r16772 not r16744.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r16754
Posted by David James <ja...@gmail.com>.
On 10/17/05, David James <ja...@gmail.com> wrote:
> On 10/17/05, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> > > On Mon, 17 Oct 2005, David James wrote:
> > >
> > > > This doesn't belong in libsvn_subr.
> > > Would libsvn_client/constructors.c be a reasonable location for the
> > > svn_client_* methods and their helper functions?
> >
> > We could add a utils.c like in libsvn_wc where we can put other small
> > functions if we need to.
>
> Ok, I'll do this. I'll also make the svn_prop_array_dup function
> public, so that it can be used by svn_client_commit_item2_dup.
Committed in r16744. I also fixed the issues with style and naming
convention. This change has been nominated for backport to 1.3.x.
Thanks for your help, Peter!
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james
Re: r16754
Posted by David James <ja...@gmail.com>.
On 10/17/05, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> > On Mon, 17 Oct 2005, David James wrote:
> >
> > > This doesn't belong in libsvn_subr.
> > Would libsvn_client/constructors.c be a reasonable location for the
> > svn_client_* methods and their helper functions?
>
> We could add a utils.c like in libsvn_wc where we can put other small
> functions if we need to.
Ok, I'll do this. I'll also make the svn_prop_array_dup function
public, so that it can be used by svn_client_commit_item2_dup.
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james
Re: r16754
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 17 Oct 2005, David James wrote:
> This doesn't belong in libsvn_subr.
Would libsvn_client/constructors.c be a reasonable location for the
svn_client_* methods and their helper functions?
We could add a utils.c like in libsvn_wc where we can put other small
functions if we need to.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r16754
Posted by David James <ja...@gmail.com>.
Hi Peter,
Thanks for your detailed comments!
On 10/17/05, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> > +svn_client_proplist_item_t *
> > +svn_client_proplist_item_dup (const svn_client_proplist_item_t *item,
> > + apr_pool_t * pool)
> > +{
> > + svn_client_proplist_item_t *new_item
> > + = apr_pcalloc (pool, sizeof (*new_item));
> > +
> > + if (item->node_name)
> > + new_item->node_name = svn_stringbuf_dup (item->node_name, pool);
> > +
> > + if (item->prop_hash)
> > + new_item->prop_hash = svn_string_hash_dup (item->prop_hash, pool);
> > +
> > + return new_item;
> > +}
> > +
>
> This doesn't belong in libsvn_subr.
Would libsvn_client/constructors.c be a reasonable location for the
svn_client_* methods and their helper functions?
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james