On Mon, Sep 28, 2009 at 4:14 AM, Hyrum K. Wright <hyrum_at_hyrumwright.org> 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. 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.)
To avoid changing the svn_delta_editor_t's close_directory API, struct
edit_baton in libsvn_wc/status.c could grow a pool field that points
to the pool that it was allocated from (a la the field in struct
dir_baton). This way, the lifetime of
eb->anchor_status->ood_last_cmt_author will correspond to that of the
edit_baton itself.
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415261
Received on 2009-11-06 22:25:22 CET