Subject: Re: [PATCH 3/3] pretty=format: Avoid some expensive calculations when not needed

From: Pierre Habouzit <[email-masked]>
Date: Wed, 07 Nov 2007 01:14:58 +0100

On Wed, Nov 07, 2007 at 12:11:12AM +0000, Pierre Habouzit wrote:
> On Tue, Nov 06, 2007 at 11:17:14PM +0000, René Scharfe wrote:
> > I haven't seen any comments on strbuf_expand.  Is it too far out?
> > Here it is again, adjusted for current master and with the changes
> > to strbuf.[ch] coming first:
> 
>   I have one.
> 
> >  strbuf.c |   22 +++++
> >  strbuf.h |    3 
> >  pretty.c |  276 ++++++++++++++++++++++++++++++++++-----------------------------
> >  3 files changed, 178 insertions(+), 123 deletions(-)
> > 
> > diff --git a/strbuf.c b/strbuf.c
> > index f4201e1..b71da99 100644
> > --- a/strbuf.c
> > +++ b/strbuf.c
> > @@ -129,6 +129,28 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
> >  	strbuf_setlen(sb, sb->len + len);
> >  }
> >  
> > +void strbuf_expand(struct strbuf *sb, const char *fmt,
> > +                   const char **placeholders, expand_fn_t fn, void *context)
> > +{
> > +	char c;
> > +	const char **p;
> > +
> > +	while ((c = *fmt++)) {
> > +		if (c != '%') {
> > +			strbuf_addch(sb, c);
> > +			continue;
> > +		}
> 
> strbuf_addch is pretty inneficient as it puts NULs each time. rather
> do that (sketchy) :
> 
> {
>     for (;;) {
>         const char *percent = strchr(fmt, '%');
>         if (!percent)
>             break;
>         strbuf_add(sb, fmt, percent - fmt);
>         fmt = percent + 1;
> 
>         /* do your stuff */
>     }
>     strbuf_addstr(sb, fmt);
> }

Or if we are at this level of micro-optimization:

{
    const char *percent = strchrnul(fmt, '%');
    while (*percent) {
        strbuf_add(sb, fmt, percent - fmt);
        fmt = percent + 1;

        /* do your stuff */

        percent = strchrnul(fmt, '%');
    }
    strbuf_add(sb, fmt, percent - fmt);
}

Which would require strchrnul, but it's trivial compat/ material for sure.

-- 
·O·  Pierre Habouzit
··O                                                [email-masked]
OOO                                                http://www.madism.org