VK Sameer wrote:
> On Wed, 2004-12-15 at 06:41, Garrett Rooney wrote:
>>>Doesn't svn_destroy_pool (subpool) have to be called to free the
>>>subpool? If so, I don't see how subpool memory would get freed if
>>>SVN_ERR causes a jump out of add_or_open_file().
>>
>>Pools are hierarchical, if you don't destroy the subpool it will get
>>cleaned up when its parent pool is eventually destroyed.
>
> OK, that's good to know. I guess I don't see the point of relying on
> that when I can just re-use an existing pool.
Sorry, I don't follow that sentence. Do you mean "... when I can use a
sub-pool that is explicitly destroyed within this function"?
> Potentially, this function
> could get called multiple times _and_ SVN_ERR could fail all those
> times. In which case, there are multiple subpools to be destroyed.
Only if the given pool has not been cleared or destroyed before it is called
again. It is the caller's duty to clean up between multiple calls, and we can
rely on it to do so. I think we can assume that the caller will clean up
quickly after a failed call, in contrast to a successful call where the result
might be in use for a long time before it is finished with.
Having said that, I wouldn't object to add_or_open_file destroying its subpool
before every error return, for consistency with the non-error case.
Note that your new statement is not the only error return from this function.
There is no point in making your new statement destroy the sub-pool and leaving
all of the existing error returns as they are. Therefore there is no need for
this change to be part of this patch: it is a separate change.
> Another alternative is to change from SVN_ERR to this (untested):
>
> svn_error_t *err;
> if (err = svn_path_check_valid (path, subpool))
> {
> svn_pool_destroy (subpool);
> return err;
> }
>
> It doesn't seem very appealing.
This idiom is used in some places. When it occurs multiple times within a
function, as it would do here, it is replaced with:
svn_error_t *err;
if (err = svn_path_check_valid (path, subpool))
goto done;
if (err = ...)
goto done;
...
done:
svn_pool_destroy (subpool);
return err;
}
This is a reasonable way of handling the situation, though a little fragile. A
more robust way is to wrap the function in a subpool-handler function that
creates and destroys the subpool, but that usually feels too cumbersome. I
think the "goto" method is the most appropriate here.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Dec 15 12:54:50 2004