On Wed, 20 Dec 2006, Erik Huelsmann wrote:
> Hi Kamesh, thanks for your review, below, I have some additional remarks.
>
> On 12/20/06, Kamesh Jayachandran <kamesh@collab.net> wrote:
> >hwright@tigris.org wrote:
> >> Modified: trunk/subversion/libsvn_client/copy.c
> >> URL:
> >http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/copy.c?pathrev=22762&r1=22761&r2=22762
> >>
> >==============================================================================
> >> --- trunk/subversion/libsvn_client/copy.c (original)
> >> +++ trunk/subversion/libsvn_client/copy.c Tue Dec 19 20:38:50 2006
> >> @@ -170,17 +205,86 @@
> >> }
> >>
> >>
> >> -struct path_driver_cb_baton
> >> +static svn_error_t *
> >> +wc_to_wc_copy(const apr_array_header_t *copy_pairs,
> >> + svn_boolean_t is_move,
> >> + svn_client_ctx_t *ctx,
> >> + apr_pool_t *pool)
> >> {
> >> - const svn_delta_editor_t *editor;
> >> - void *edit_baton;
> >> - svn_node_kind_t src_kind;
> >> + int i;
> >> + apr_pool_t *subpool = svn_pool_create(pool);
> >>
> >Do you intend to use this subpool in both the for loops. But it does not
> >seem so.
>
> Also, there are 2 naming conventions: 'subpool' if the pool is used to
> clear excessive resource useage in the function, for example,
> allocation of (large) file buffers, open files or xml parsers. When
> the pool is destroyed at the end of the function, those resources will
> be freed.
>
> The other is 'iterpool' which is used in iterations to allocate
> resources on a per-iteration basis. These are cleared between
> iterations. Sometimes there is an iterpool1 and iterpool2, which get
> cleared alternatingly when resources are needed in the next iteration.
>
> >If you don't want to use subpool in the first loop, why unneccarily
> >clear it in the first loop.
>
> But he does :-)
I've freed up the obviously temporary resources and renamed "subpool"
to "iterpool" in r22765 (along with some other tweaks). There some
allocations taking place in the svn_client__copy_pair_t elements in
the COPY_PAIRS array passed to wc_to_wc_copy(), but I kept those in
POOL since we're modifying fields on our inputs.
...
> >> --- trunk/subversion/tests/cmdline/copy_tests.py (original)
> >> +++ trunk/subversion/tests/cmdline/copy_tests.py Tue Dec 19
...
> >There should be no space between function name 'argument paren'.
Fixed in r22766.
- application/pgp-signature attachment: stored
Received on Wed Dec 20 15:13:10 2006