[PATCH] Issue #1310: Clear iteration pools at start of loop

[Julian Foad <ju...@btopenworld.com>]

I happened to notice this open issue that is apparently a very easy task.

Issue #1310: Clear iteration pools at the beginning of each iteration rather 
than the end, and destroy them afterwards, as per our policy.

Michael Price attached the latest revision of his patch to the issue last July. 
  I have updated it and reviewed it carefully.  I noticed a few oddities:

+ subversion/libsvn_client/commit.c (svn_client_import):
   The patch adds a 'destroy' at the end of the function.  Does that invalidate 
'commit_info' which seems to be allocated in the subpool and then returned to 
the caller?  An import seems to work OK, and passes 'make check', even if I 
'clear' just before 'destroy', but is it right?  I think I will remove it from 
the patch if I don't find out for sure that it's safe.

+ subversion/libsvn_fs_fs/fs_fs.c (fetch_all_changes):
   Currently, 'iterpool' is cleared part way through the loop body so that data 
can be read into it before the loop and at the end of the loop body.  I intend 
to leave this as it is.

+ Two loops do not use their sub-pool:
     subversion/libsvn_client/add.c (svn_client_mkdir): subpool
     subversion/mod_dav_svn/repos.c (dav_svn_do_walk): params_subpool
I intend to fix these in a separate patch and not touch them in this one. (Use 
the subpool in the first case, where it is currently using the main pool. 
Remove the subpool in the second case, because a pool is not needed.)

Any comments on these oddities (especially the first one) or the patch itself 
before I commit it?

- Julian


Log message:
[[[
Issue #1310: Clear iteration pools at the beginning of each iteration rather
than the end, and destroy them afterwards, as per our policy.

Patch from Michael Price.
]]]

Re: [PATCH] Issue #1310: Clear iteration pools at start of loop

[kf...@collab.net]

Julian Foad <ju...@btopenworld.com> writes:
> + subversion/libsvn_client/commit.c (svn_client_import):
>    The patch adds a 'destroy' at the end of the function.  Does that
> invalidate 'commit_info' which seems to be allocated in the subpool
> and then returned to the caller?  An import seems to work OK, and
> passes 'make check', even if I 'clear' just before 'destroy', but is
> it right?  I think I will remove it from the patch if I don't find out
> for sure that it's safe.

Agreed.  Maybe we should just be passing 'pool' instead of 'subpool'
to get_ra_editor().  In the current code, the effect would be the
same, and if we were to change svn_client_import() to destroy the
subpool at the end, I don't see how it could possibly be correct
w.r.t. the allocation of commit_items (at least, tracing the code
through indicates that commit_items are allocated in the explicit
pool, not the session pool or some other sneaky way).

-K

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org