[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: rev 1912 - trunk/subversion/include trunk/subversion/libsvn_subr trunk/subversion/libsvn_repos

From: Ben Collins-Sussman <sussman_at_collab.net>
Date: 2002-05-10 00:12:43 CEST

Greg Stein <gstein@lyra.org> writes:

>
> use apr_size_t
>
> The cast is unnecessary. keybuf is already a 'char *'
>
>
> The cast isn't needed.
>
> Also note that you're passing svn_pack_bytestring() as a pack_func, but that
> returns a stringbuf, not a plain string (Badness!). But I've gotta ask: what
> the heck is this "pack function" *for* ? You've got a buffer and length, so
> why not just construct an svn_string_t yourself? For example:

All of this code was simply ripped out of libsvn_subr/hashdump.c,
which was the -first- code ever written for subversion, about two
years ago.

IIRC, the code used to read/write hashtables to disk require 'pack'
and 'unpack' routines. This was done so that any old data-type could
be saved and *resurrected* from the hashdump file. Kind of like
object serialization, or python's 'pickle' thing. The 'unpack'
routine takes some (void *) and turns it into a printable bytestring
to put in the file. The 'pack' routine takes a bytestring read from
the file, and returns a (void *) to be stored as a hash value.

Of course, the only existing pack/unpack routines (at the moment) are
for stringbufs, so it's kind of silly to use this generalization when
reading properties in our dumpstream. I mean, they're props, so we
*know* they're svn_string_t values. No need for generalization here.
I'll simplify it.

> > + if (text_stream != NULL)
> > + {
> > + char buffer[SVN_STREAM_CHUNK_SIZE];
> > + apr_size_t buflen = SVN_STREAM_CHUNK_SIZE;
>
> WOAH! You simply cannot allocate that on the stack. You're looking at over
> a 100k(!)
>
> I'd suggest allocating the block once at the start of the parse, and simply
> reusing it within the parse_content_block() function. e.g. have some kind a
> "parse_ctx" structure which can hold that buffer (and maybe a couple
> stringbufs for the key/value buffers which can be sized properly and reused).

<blush>

Fixed, as you suggest.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri May 10 00:16:21 2002

This is an archived mail posted to the Subversion Dev mailing list.