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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]