Eric Gillespie wrote:
> I've tested fairly extensively, and the test suite passes.
Excellent.
> If there are no objections, i'll commit it.
I'm no expert, but I have a concern...
> === subversion/libsvn_fs_fs/dag.c
> ==================================================================
> --- subversion/libsvn_fs_fs/dag.c (revision 1910)
> +++ subversion/libsvn_fs_fs/dag.c (local)
> @@ -23,6 +23,7 @@
> #include "svn_error.h"
> #include "svn_fs.h"
> #include "svn_props.h"
> +#include "svn_pools.h"
>
> #include "dag.h"
> #include "err.h"
> @@ -236,29 +237,34 @@
> svn_fs_t *fs = svn_fs_fs__dag_get_fs (node);
> dag_node_t *this_node;
> svn_boolean_t done = FALSE;
> + apr_pool_t *iterpool;
>
> + iterpool = svn_pool_create (pool);
> this_node = node;
> while ((! done) && this_node)
> {
> node_revision_t *noderev;
>
> + svn_pool_clear (iterpool);
> +
> /* Get the node revision for THIS_NODE so we can examine its
> predecessor id. */
> - SVN_ERR (get_node_revision (&noderev, this_node, pool));
> + SVN_ERR (get_node_revision (&noderev, this_node, iterpool));
>
> /* If THIS_NODE has a predecessor, replace THIS_NODE with the
> precessor, else set it to NULL. */
> if (noderev->predecessor_id)
> SVN_ERR (svn_fs_fs__dag_get_node (&this_node, fs,
> - noderev->predecessor_id, pool));
> + noderev->predecessor_id, iterpool));
Here, this_node is allocated in iterpool. On the next iteration of the loop,
iterpool is cleared and then this_node is used (not just in the non-NULL test
but in the call to get_node_revision). That looks wrong.
> else
> this_node = NULL;
>
> /* Now call the user-supplied callback with our predecessor
> node. */
> if (callback)
> - SVN_ERR (callback (baton, this_node, &done, pool));
> + SVN_ERR (callback (baton, this_node, &done, iterpool));
> }
> + apr_pool_destroy (iterpool);
>
> return SVN_NO_ERROR;
> }
> === subversion/libsvn_fs_fs/tree.c
> ==================================================================
> --- subversion/libsvn_fs_fs/tree.c (revision 1910)
> +++ subversion/libsvn_fs_fs/tree.c (local)
[...]
> @@ -1724,6 +1728,7 @@
> }
>
> /* For each entry E in source but not in ancestor */
> + svn_pool_clear (iterpool);
That line appears to be redundant, as the pool is cleared a few lines further
on, inside the next loop. Maybe you wanted to be sure it is cleared here as
soon as the first loop has finished with it, as an idiom to try to catch
accidental use of iterpool outside the loop. I'm not sure that that is
worthwhile, but if you think it is, then that's OK with me.
> for (hi = apr_hash_first (pool, s_entries);
> hi;
> hi = apr_hash_next (hi))
> @@ -1734,6 +1739,8 @@
> apr_ssize_t klen;
> svn_boolean_t s_ancestorof_t = FALSE;
>
> + svn_pool_clear (iterpool);
> +
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Feb 27 22:56:52 2005