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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 16, 2007, 00:57 UTC
Message-ID: <7vwsur77g7.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20070915141335.ECB4B4C152@madism.org>

Pierre Habouzit <madcoder@debian.org> writes:

Show 5 quoted lines

> * 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.

Show 6 quoted lines

> -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)
        {
        	...

Show 5 quoted lines

> +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.

> +	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.

By the way, this is the kind of situation I wish everybody wrote their comparison in textual order. I had to draw a picture like this to see what was going on.

        sb->buf
        xxxxxxxxxxxzzzzzxxxxxxxxxxx\0
        ^                         ^
        0                   sb->len
                   ^    ^               ^    ^        
                   pos  pos+len         pos  pos+len  
                   v                    v
                   yyyyyyyyyy           yyyyyyyyyy    
                             ^                    ^   
                             dlen                 dlen
                   -------------        --------------
                   pos < sb->len        sb->len < pos