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. 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=2401189
Received on 2009-09-28 14:14:31 CEST