From: Junio C Hamano Date: Sun, 16 Sep 2007 00:57:44 GMT Subject: Re: [PATCH] New strbuf APIs: splice and embed. Message-ID: <7vwsur77g7.fsf@gitster.siamese.dyndns.org> In-Reply-To: <20070915141335.ECB4B4C152@madism.org> Pierre Habouzit 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. > -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) { ... > +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