On Sun, Sep 16, 2007 at 12:57:44AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@xxxxxxxxxx> 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@xxxxxxxxxx
OOO http://www.madism.org
Attachment:
pgpTygyqO8CAb.pgp
Description: PGP signature
