On Mon, 2009-09-28 at 08:14 -0400, Hyrum K. Wright wrote:
> On Sep 28, 2009, at 6:36 AM, Julian Foad wrote:
>
> > On Sat, 2009-09-26, Hyrum K. Wright wrote:
> >> On Sep 26, 2009, at 1:06 PM, Martin Hauner wrote:
> >>> there is one place in moving libsvn_wc/status.c to result_pool/
> >>> scratch_pool where I'm not sure what to do.
> >>>
> >>> status.c has the following function:
> >>>
> >>> static svn_error_t *
> >>> close_directory(void *dir_baton,
> >>> apr_pool_t *scratch_pool)
> >>>
> >>>
> >>> All pool usages are fine with scratch_pool but a single line looks
> >>> like a
> >>> result_pool:
> >>>
> >>> (currently line 1865)
> >>>
> >>> eb->anchor_status->ood_last_cmt_author =
> >>> apr_pstrdup(pool, db->ood_last_cmt_author);
> >
> > If that allocation is being used to store results for use after the
> > function returns, then the pool is a "result pool". There is no harm
> > in
> > a result pool also being used to hold some scratch data, so the
> > solution
> > is to change the function's prototype to
> >
> > static svn_error_t *
> > close_directory(void *dir_baton,
> > apr_pool_t *result_pool)
> >
> > (Or just leave it as "pool" and add a comment explaining why.)
>
> I haven't looked at the docs for this particular function, but our
> pattern for callback functions it to always provide a scratch_pool,
> and require providers of the callback to include any result_pool as
> part of their provided closure.
Ahh... yes, svn_delta_editor_t's doc string clearly implies what you
say. It looks like there was already a pool usage bug here, and that
data should instead be stored in a persistent pool.
Thanks, and sorry for the bad advice.
- Julian
> If close_directory() follows this
> pattern (and I see no reason why it should not), renaming the argument
> would be misleading at best. (It also makes me wonder why we're
> dup'ing the value into a supposed scratch_pool in the first place.)
>
> -Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2401212
Received on 2009-09-28 15:13:35 CEST