On Sun, Aug 16, 2009 at 04:08:51PM +0200, Martin Hauner wrote:
> here is a small patch for a single function to see if this is what you like to
> see :)
>
> I wonder if it as easy as you think. For each pool usage I have to decide if it
> should use result or scratch. I can get this wrong...
There is a simple rule to this. If you are allocating memory for
an output parameter, i.e. a value that is used by callers of the
function, use the result pool. Else, the value won't be used after
the function returns, so it can be put into the scratch pool.
> It won't be easy for a reviewer to catch an error, so I guess I better ask if in
> doubt. Let's hope I don't have to ask too many times. :)
>
>
> --
> Martin
>
> Subcommander 2.0.0 Beta 4 - http://subcommander.tigris.org
> a Win32/Unix/MacOSX subversion GUI client & diff/merge tool.
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2384055
> [[
> Use result_pool/scratch_pool paradigm in libsvn_wc.
>
> * subversion/libsvn_wc/wc.h
> (svn_wc__ensure_directory): renamed pool to result_pool, added scratch_pool
> * subversion/libsvn_wc/util.c
> (svn_wc__ensure_directory): adjusted pool usages.
> ]]
>
> Index: /Users/hauner/Development/subversion/svn-trunk2/subversion/libsvn_wc/util.c
> ===================================================================
> --- /Users/hauner/Development/subversion/svn-trunk2/subversion/libsvn_wc/util.c (revision 38761)
> +++ /Users/hauner/Development/subversion/svn-trunk2/subversion/libsvn_wc/util.c (working copy)
> @@ -43,11 +43,12 @@
>
> svn_error_t *
> svn_wc__ensure_directory(const char *path,
> - apr_pool_t *pool)
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> {
> svn_node_kind_t kind;
>
> - SVN_ERR(svn_io_check_path(path, &kind, pool));
> + SVN_ERR(svn_io_check_path(path, &kind, result_pool));
This looks wrong. kind is a local variable, and this function
does not return it to the caller. So we'll have to use a scratch pool here.
>
> if (kind != svn_node_none && kind != svn_node_dir)
> {
> @@ -56,12 +57,12 @@
> Might happen if there's a file in the way, for example. */
> return svn_error_createf(APR_ENOTDIR, NULL,
> _("'%s' is not a directory"),
> - svn_dirent_local_style(path, pool));
> + svn_dirent_local_style(path, scratch_pool));
I'm not really sure about this one. You are returning an error,
and which pool to use depends on the caller's behaviour.
Maybe there is a convention for this, I'm not sure. Greg?
I'd probably chose the safe side and use the result pool here.
But read on...
> }
> else if (kind == svn_node_none)
> {
> /* The dir doesn't exist, and it's our job to change that. */
> - svn_error_t *err = svn_io_dir_make(path, APR_OS_DEFAULT, pool);
> + svn_error_t *err = svn_io_dir_make(path, APR_OS_DEFAULT, result_pool);
svn_io_dir_make does not allocate and return anything to us here,
so scratch_pool is more appropriate.
>
> if (err && !APR_STATUS_IS_ENOENT(err->apr_err))
> {
> @@ -77,7 +78,7 @@
> /* Okay, so the problem is a missing intermediate
> directory. We don't know which one, so we recursively
> back up one level and try again. */
> - const char *shorter = svn_dirent_dirname(path, pool);
> + const char *shorter = svn_dirent_dirname(path, scratch_pool);
This use is correct. Our caller doesn't care about the value of 'shorter'.
>
> /* Clear the error. */
> svn_error_clear(err);
> @@ -90,8 +91,12 @@
> }
> else /* We have a valid path, so recursively ensure it. */
> {
> - SVN_ERR(svn_wc__ensure_directory(shorter, pool));
> - return svn_wc__ensure_directory(path, pool);
> + SVN_ERR(svn_wc__ensure_directory(shorter,
> + result_pool,
You've allocated shorter in scratch pool. ensure_directory does not
return anything here that we need. So pass scratch_pool for both pools.
> + scratch_pool));
> + return svn_wc__ensure_directory(path,
> + result_pool,
> + scratch_pool);
Same here.
And after looking through all this, it looks like this function does not
need a result pool at all. All it does it either create a directory,
or throw an error if a file exists where the caller expects a directory.
There are no output parameters, except for the error, but I'm not sure
if those should go into the result pool. So you could probably simply
rename the existing 'pool' to scratch_pool and be done with it.
> }
> }
>
> Index: /Users/hauner/Development/subversion/svn-trunk2/subversion/libsvn_wc/wc.h
> ===================================================================
> --- /Users/hauner/Development/subversion/svn-trunk2/subversion/libsvn_wc/wc.h (revision 38761)
> +++ /Users/hauner/Development/subversion/svn-trunk2/subversion/libsvn_wc/wc.h (working copy)
> @@ -249,7 +249,9 @@
> * If this section gets big, move it all out into a new util.h file. */
>
> /* Ensure that DIR exists. */
> -svn_error_t *svn_wc__ensure_directory(const char *path, apr_pool_t *pool);
> +svn_error_t *svn_wc__ensure_directory(const char *path,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool);
When adding new parameters, please also update the docstring to explain
what they are used for.
Stefan
>
> /* Baton for svn_wc__compat_call_notify_func below. */
> typedef struct svn_wc__compat_notify_baton_t {
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2384064
Received on 2009-08-16 16:28:45 CEST