On Thu, Sep 06, 2007 at 05:59:29PM +0000, Kristian Høgsberg wrote:
> On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
> > + strbuf_grow(sb, len);
> > + strbuf_addf(sb, "%u %s=", len, keyword);
> > + strbuf_add(sb, value, valuelen);
> > + strbuf_addch(sb, '\n');
> > }
>
> This entire function can be collapsed to just:
>
> strbuf_addf(sb, "%u %s=%.*s\n", len, keyword, valuelen, value);
yes, but it's less efficient, because %.*s says that sprintf must copy
at most valuelen bytes from value, but it still has to stop if it finds
a \0 before. And the strbuf_grow has sense because the extend policy at
snprintf time is optimistic: we try to write, and if it didn't fit, we
try again. So there is a huge benefit if we have a clue of the final
size.
I would not change a thing.
> > + strbuf_init(&ext_header);
>
> Just use your STRBUF_INIT macro?
Many people dilike it, I'm not sure it's a good idea either, and the
performance hit should be negligible, if it's not, then we can still
make the _init() function an inline.
> > if (ext_header.len > 0) {
> > write_entry(sha1, NULL, 0, ext_header.buf, ext_header.len);
> > - free(ext_header.buf);
> > }
>
> Remove excess braces?
bah, I don't like to strip braces so I won't do that, else you end up
with stupidities like:
if (foo)
// bar();
do_some_very_important_stuff();
Call me paranoid but well, it saved me so many times ...
> > - memcpy(path.buf, base, baselen);
> > - memcpy(path.buf + baselen, filename, filenamelen);
> > - path.len = baselen + filenamelen;
> > - path.buf[path.len] = '\0';
> > + strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
> > + strbuf_reset(&path);
>
> Does strbuf_reset() do anything here?
>
> > + strbuf_add(&path, base, baselen);
Yes _reset() sets length to 0. so the add here will write at the start
of the buffer again. It definitely is important !
--
·O· Pierre Habouzit
··O madcoder@xxxxxxxxxx
OOO http://www.madism.org
Attachment:
pgpFtLt7giDZ1.pgp
Description: PGP signature
