On Aug 9, 2005, at 6:15 PM, Alexander Thomas wrote:
> + /* Get lock. */
> + SVN_ERR (svn_ra_get_locks (ra_session, locks, "", subpool));
> +
> + new_locks = apr_hash_make (pool);
> + for (hi = apr_hash_first (subpool, *locks);
> + hi;
> + hi = apr_hash_next (hi))
> + {
> + const void *key;
> + void *val;
> + const char *newkey;
> +
> + apr_hash_this (hi, &key, NULL, &val);
> + newkey = svn_path_is_child (svn_path_canonicalize (rel_path,
> subpool),
> + svn_path_canonicalize (key,
> subpool),
> + pool);
> + if (newkey)
> + apr_hash_set (new_locks, newkey, APR_HASH_KEY_STRING, val);
> + }
> +
> + apr_pool_destroy (subpool);
Oops, this should svn_pool_destroy().
> + *locks = new_locks;
> +
>
I'm not going to ask you to resubmit a new patch just for this, but I
want to make it clear what the point of the subpool is. I think
you're still a tiny bit unclear on the usage.
Generally, one makes a subpool when a loop is going to use temporary
memory. That means that the body of the loop should (1) allocating
all temporary objects in the subpool, (2) clearing the subpool on
every iteration.
What you're doing above doesn't match that pattern. You're doing
(1), but you're not doing (2). I understand that you're not doing
(2) because you're using the subpool for the hash iteration. Don't
do that! apr_hash_first() should be using the main pool to loop, and
the subpool should *only* be used within the body of the loop. Then
it's safe to clear it on each iteration.
In any case, I'll tweak myself... no need to resubmit.
I'm going to actually apply the patch now and play around with it. :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Aug 10 15:21:05 2005