I wrote:
> This is exactly the conclusion Ben came to -- in fact, he started down
> the path of making the big "change set_prop to take const
> svn_string_t" road, and backed out after realizing it was going to use
> up the whole day and more.
>
> If your comment got removed from the code, it was an accident; I know
> from conversation that he meant to leave it in after aborting the
> larger change. There is also issue #406, so this is not going to get
> forgotten about.
Hmm, I take that back, having read the log message & change more
carefully now -- in fact, Ben, why are we changing the way the
stringbuf's are built at all? The old way was faster and didn't do
any extra heap allocation...
-K
Greg Stein <gstein@lyra.org> writes:
> On Thu, Oct 18, 2001 at 01:13:57PM -0500, sussman@tigris.org wrote:
> >...
> > Cleanup: make 'relative' into a const char *. Remove gstein's
> > complaint. :-)
> >...
> > +static svn_error_t *bump_resource(merge_ctx_t *mc,
> > + const char *path,
> > + char *vsn_url)
> > {
> > - svn_stringbuf_t path_str = { 0 };
> > - svn_stringbuf_t vsn_url_str = { 0 };
> > + svn_stringbuf_t *path_str;
> > + svn_stringbuf_t *vsn_url_str;
> >
> > /* import case. just punt for now. */
> > if (mc->close_commit == NULL)
> > @@ -136,30 +138,21 @@
> > if (! apr_hash_get (mc->valid_targets, path, APR_HASH_KEY_STRING))
> > return NULL;
> >
> > - /* ### damned callbacks take svn_stringbuf_t even though they don't plan
> > - ### to change the values whatsoever... */
> > /* set up two svn_stringbuf_t values around the path and vsn_url. */
> > - path_str.data = path;
> > - path_str.len = strlen(path);
> > - path_str.blocksize = path_str.len + 1;
> > - path_str.pool = mc->pool;
> > -
> > - vsn_url_str.data = vsn_url;
> > - vsn_url_str.len = strlen(vsn_url);
> > - vsn_url_str.blocksize = vsn_url_str.len + 1;
> > - vsn_url_str.pool = mc->pool;
> > -
> > + path_str = svn_stringbuf_create (path, mc->pool);
> > + vsn_url_str = svn_stringbuf_create (vsn_url, mc->pool);
>
> The above change does nothing at all to remove my complaint. All that your
> change does is to move some stack-based structures to the heap (an extra
> allocation). In other words, marginally worse than before.
>
> > +
> > /* store the version URL */
> > - SVN_ERR( (*mc->set_prop)(mc->close_baton, &path_str,
> > - mc->vsn_url_name, &vsn_url_str) );
> > + SVN_ERR( (*mc->set_prop)(mc->close_baton, path_str,
> > + mc->vsn_url_name, vsn_url_str) );
>
> The *problem* is that set_prop takes an svn_stringbuf_t. It has absolutely
> no need to do that since it isn't going to change the contents. That is
> exactly what the bitch in my comment said.
>
> set_prop should take "const svn_string_t *" or simply "const char *".
>
> But if we aren't going to make that change now, then my bitches should stay
> in the code so that I'll find 'em again later and fix the darned set_prop at
> some point.
>
> Cheers,
> -g
>
> --
> Greg Stein, http://www.lyra.org/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:36:45 2006