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

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

On Mon, Mar 17, 2014 at 11:07 AM, Michael Haggerty [off-list ref] wrote:

quoted

On 03/17/2014 07:33 AM, Junio C Hamano wrote:

quoted

Junio C Hamano [off-list ref] writes:

quoted

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