Re: svn commit: r8223 - trunk/subversion/mod_dav_svn

cmpilato@tigris.org writes:

> Author: cmpilato
> Date: Fri Jan 9 13:22:33 2004
> New Revision: 8223
>
> Modified:
> trunk/subversion/mod_dav_svn/update.c
> Log:
> * subversion/mod_dav_svn/update.c
> (dav_svn__update_report): Use a subpool during the REPORT parsing.
> This wonderful memory leak fix provided (mostly) by Tobias
> Ringstrom <tobias@ringstrom.mine.nu>.
>
> Modified: trunk/subversion/mod_dav_svn/update.c
> ==============================================================================
> --- trunk/subversion/mod_dav_svn/update.c (original)
> +++ trunk/subversion/mod_dav_svn/update.c Fri Jan 9 13:22:33 2004

> @@ -1251,6 +1252,9 @@
> for (child = doc->root->first_child; child != NULL; child = child->next)
> if (child->ns == ns)
> {
> + /* Clear our subpool. */
> + svn_pool_clear(subpool);

That triggers an alert in my "redundant comment filter". The desire
to add comments is good, but if the comment simply duplicates the code
then the overall effect is negative. There are lots of places in the
existing code that have comments that are equally redundant, but
that's not a reason to add more.

> +
> if (strcmp(child->name, "entry") == 0)
> {
> const char *path;
> @@ -1284,14 +1288,14 @@
> }
>
> /* get cdata, stipping whitespace */
> - path = dav_xml_get_cdata(child, resource->pool, 1);
> + path = dav_xml_get_cdata(child, subpool, 1);
>
> if (! linkpath)
> serr = svn_repos_set_path(rbaton, path, rev,
> - start_empty, resource->pool);
> + start_empty, subpool);
> else
> serr = svn_repos_link_path(rbaton, path, linkpath, rev,
> - start_empty, resource->pool);
> + start_empty, subpool);
> if (serr != NULL)
> {
> svn_error_clear(svn_repos_abort_report(rbaton, resource->pool));

Now this case is more interesting, it's the ideal place for a comment
explaing why the subpool is *not* being used. Or did you just miss
this one?

(While svn_repos_abort_report doesn't use the pool at present it might
use it one day.)

> @@ -1411,6 +1412,9 @@
> "and response generation for the update "
> "report.");
> }
> +
> + /* Destroy our subpool. */
> + svn_pool_destroy(subpool);

Another one.

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org