Re: [PATCH 1/3] refactor dir_add_name

From: Jeff King <peff@peff.net>
Date: Jun 11, 2007, 19:46 UTC
Message-ID: <20070611194651.GA15309@coredump.intra.peff.net>
In-Reply-To: <7vk5uaqx3q.fsf@assigned-by-dhcp.pobox.com>

On Mon, Jun 11, 2007 at 09:23:05AM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> > +#define alloc_grow(x, nr, alloc) do { \
> > +	if(nr >= alloc) { \
> > +		alloc = alloc_nr(alloc); \
> > +		x = xrealloc((x), alloc * sizeof(*(x))); \
> > +	} \
> > +} while(0)
> 
> worry a bit about macro safety.  I think the presence of an

Agreed. The multiple evaluation of 'x' means that growing foo[i++] will cause quite a confusing bug. I would prefer it as an inline, but at least the sizeof requires macro magic. Not to mention the ugliness of moving all those lvalues as pointers. But we could do something like (totally untested):

#define alloc_grow(x, nr, alloc) \
  alloc_grow_helper(&(x), nr, &(alloc), sizeof(*(x)))

inline
void alloc_grow_helper(void **x, int nr, int *alloc, int size)
{
  if (nr >= *alloc) {
    *alloc = alloc_nr(*alloc);
    *x = xrealloc(*x, *alloc * size);
  }
}

Horribly ugly (I'm seeing stars!) but probably a bit safer in the long run, and nobody needs to look at it most of the time. :)

What do you think?

> assignment to alloc and x would make sure we would catch an
> error to pass non lvalue as 'alloc' and 'x', so it may be Ok as

Even lvalues can have side effects, so multiple evaluation is still a problem.

> is.  A comment before the macro, and a space between 'if' and
> opening parenthesis, would be good things to have.

Yes, sorry, my fingers are always forgetting the git whitespace conventions. I (or Jonas) will submit a better commented version, but do let us know which version you like.

-Peff