Re: [PATCH] New strbuf APIs: splice and embed.

[Pierre Habouzit <madcoder@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]

On Sun, Sep 16, 2007 at 12:57:44AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > * strbuf_splice replace a portion of the buffer with another.
> > * strbuf_embed replace a strbuf buffer with the given one, that should be
> >   malloc'ed. Then it enforces strbuf's invariants. If alloc > len, then this
> >   function has negligible cost, else it will perform a realloc, possibly
> >   with a cost.
> 
> "embed" does not sound quite right, does it?  It is a reverse
> operation of strbuf_detach() as far as I can tell.

  Well I don't like either, and indeed strbuf_attach() seems better.

> > -void strbuf_rtrim(struct strbuf *sb)
> > -{
> > +void strbuf_rtrim(struct strbuf *sb) {
> >  	while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))
> >  		sb->len--;
> >  	sb->buf[sb->len] = '\0';
> 
> This is changing the style in the wrong direction, isn't it?  We
> start our functions like this:
> 
> 	type name(proto)
>         {

  Well that's what I usually do for my code, but I thought git was
putting the opening brace on the same line, my bad.

> > +void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
> > +				   const void *data, size_t dlen)
> > +{
> > +	if (pos + len < pos)
> > +		die("you want to splice outside from the buffer");
> 
> That is a funny error message for an integer wrap-around check.

  right ;)

> 
> > +	if (pos > sb->len)
> > +		pos = sb->len;
>
> Shouldn't this be flagged as a programming error?
> 
> > +	if (pos + len > sb->len)
> > +		len = sb->len - pos;
> 
> Likewise.

  I just attached the same semantics that was chose for strbuf_insert
when the insertion position is outside from the buffer. Though I can
enforce those to stay inside the buffer and just die(). I don't care
much I shall say. Maybe it hides some programmers being sloppy, hence is
a bad semantics. I'll propose a patch where the check die()s instead of
"fixing" the values.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]