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
Received on Mon Oct 17 11:16:43 2005