Re: [PATCH] add pool pointer to svn_string_t

On Wed, Oct 11, 2000 at 09:10:35PM +0100, Joe Orton wrote:
> On Tue, Oct 10, 2000 at 06:39:10PM -0400, Greg Hudson wrote:
> > I support this change, but it might be nice to hear the opion of one
> > of the original Subversion people for such a basic change.

I dunno if I count as an "original" Subversion person :-) ... but my basic
thought is that adding the pool is a Good Thing.

However, I'll take this opportunity to make a few additional statements:

*) svn_string was originally conceived as a "smart buffer for manipulating
   strings", with minimal realloc

*) we have 54 calls to svn_string_append* in the code. 40 in our code, 14 in
   the test programs.

*) many of those calls are sequences: see
       libsvn_subr/xml.c::svn_xml_make_close_tag

   implying that the number of *code points* where append behavior is needed
   is even rarer than the 40 would indicate.

*) I found 263 instances of '>data' in the code (presumably, the majority
   are derefs of svn_string.data; and a quick perusal seems to confirm
   this). 195 in our code, the rest in the test programs.

*) This tells me that we are not using the buffering features of the strings
   nearly as much as we're simply using them as containers.

*) why wrap a container around a "const char *" ??

IMO, we should switch to passing "const char *" more often. Where we need
the buffering features of svn_string, we should use a new svn_buffer type
(stripped down from today's svn_string).

Note also that some of that buffering is meaningless in the presence of
pools. So what if dup a little extra storage? It's only temporary.

Lastly... consider the code simplification. Go back to to the function
referenced above (svn_xml_make_close_tag). I could easily see that whole
function as:

       const char *svn_xml_make_close_tag(apr_pool_t *pool,
                                          const char *tagname)
       {
         return apr_psprintf(pool, "</%s>", tagname);
       }

What could be simpler? :-)

Summary: torch all uses of svn_string. Replace a few with svn_buffer, which
is a derivative work from svn_string. And yes: svn_buffer would record a
pool for its use.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/