Re: [PATCH] mv: prevent mismatched data when ignoring errors.

From: Jeff King <peff@peff.net>
Date: Mar 17, 2014, 22:04 UTC
Message-ID: <20140317220427.GA19300@sigill.intra.peff.net>
In-Reply-To: <CAPig+cTs23=j_1OsB4FUUb1PZWubhad+XBCa1iEx4jChZE2x4w@mail.gmail.com>

On Mon, Mar 17, 2014 at 03:06:02PM -0400, Eric Sunshine wrote:

Show 19 quoted lines

> On Mon, Mar 17, 2014 at 11:07 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> > On 03/17/2014 07:33 AM, Junio C Hamano wrote:
> >> Junio C Hamano <gitster@pobox.com> 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