From: Jeff King <peff@peff.net>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Michael Haggerty <mhagger@alum.mit.edu>,
Junio C Hamano <gitster@pobox.com>,
Thomas Rast <tr@thomasrast.ch>,
"brian m. carlson" <sandals@crustytoothpaste.net>,
Git List <git@vger.kernel.org>,
Jens Lehmann <Jens.Lehmann@web.de>,
John Keeping <john@keeping.me.uk>,
Guillaume Gelin <contact@ramnes.eu>
Subject: Re: [PATCH] mv: prevent mismatched data when ignoring errors.
Date: Mon, 17 Mar 2014 18:04:27 -0400 [thread overview]
Message-ID: <20140317220427.GA19300@sigill.intra.peff.net> (raw)
In-Reply-To: <CAPig+cTs23=j_1OsB4FUUb1PZWubhad+XBCa1iEx4jChZE2x4w@mail.gmail.com>
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@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
next prev parent reply other threads:[~2014-03-17 22:04 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-08 16:23 git 1.9.0 segfault Guillaume Gelin
2014-03-08 16:46 ` brian m. carlson
2014-03-08 18:12 ` John Keeping
2014-03-08 18:35 ` [PATCH] builtin/mv: fix out of bounds write John Keeping
2014-03-08 19:15 ` brian m. carlson
2014-03-08 19:29 ` [PATCH v2] " John Keeping
2014-03-08 19:21 ` [PATCH] mv: prevent mismatched data when ignoring errors brian m. carlson
2014-03-11 1:56 ` Jeff King
2014-03-11 2:00 ` brian m. carlson
2014-03-11 21:45 ` Junio C Hamano
2014-03-12 23:21 ` brian m. carlson
2014-03-15 16:05 ` Thomas Rast
2014-03-16 2:00 ` Jeff King
2014-03-16 21:20 ` Junio C Hamano
2014-03-17 6:33 ` Junio C Hamano
2014-03-17 15:07 ` Michael Haggerty
2014-03-17 19:06 ` Eric Sunshine
2014-03-17 22:04 ` Jeff King [this message]
2014-03-18 22:31 ` Junio C Hamano
2014-03-15 18:56 ` [PATCH v2] " brian m. carlson
2014-03-16 2:00 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140317220427.GA19300@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=Jens.Lehmann@web.de \
--cc=contact@ramnes.eu \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=john@keeping.me.uk \
--cc=mhagger@alum.mit.edu \
--cc=sandals@crustytoothpaste.net \
--cc=sunshine@sunshineco.com \
--cc=tr@thomasrast.ch \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).