On Mon, Mar 17, 2014 at 03:06:02PM -0400, Eric Sunshine wrote:
> On Mon, Mar 17, 2014 at 11:07 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
> > On 03/17/2014 07:33 AM, Junio C Hamano wrote:
> >> Junio C Hamano <gitster@xxxxxxxxx> writes:
> >>
> >>> Would it make sense to go one step further to introduce two macros
> >>> to make this kind of screw-up less likely?
> > potential callers that I noticed, ALLOC_GROW() was used immediately
> > before making space in the array for a new element. So I suggest
> > something more like
> >
> > +#define MOVE_DOWN(array, nr, at, count) \
> > + memmove((array) + (at) + (count), \
> > + (array) + (at), \
> > + sizeof((array)[0]) * ((nr) - (at)))
>
> Each time I read these, my brain (for whatever reason) interprets the
> names UP and DOWN opposite of the intended meaning, which makes them
> confusing. Perhaps INSERT_GAP and CLOSE_GAP would avoid such problems,
> and be more consistent with Michael's proposed ALLOC_INSERT_GAP.
Yeah, the UP/DOWN are very confusing to me. Something like SHRINK/EXPAND
(with the latter handling the ALLOC_GROW for us) makes more sense to me.
Those terms do not explicitly specify that we are doing it in the middle
(whereas GAP does), but I think it is fairly obvious from the parameters
what each is used for.
Side note: I had _almost_ added something to my original email
suggesting more use of macros to embody common idioms. For example, in
the vast majority of malloc cases, you could using something like:
#define ALLOC_OBJS(x,n) do { x = xmalloc(sizeof(*x) * (n)); } while(0)
#define ALLOC_OBJ(x) ALLOC_OBJS(x,1)
That eliminates a whole possible class of errors. But it's also
un-idiomatic as hell, and the resulting confusion can cause its own
problems. So I refrained from suggesting it.
I think as long as a macro is expressing a more high-level intent,
though, paying that cost can be worth it. By itself, wrapping memmove
to use sizeof(*array) does not seem all that exciting. But wrapping a
few specific cases like shrink/expand probably does make the code more
readable.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html