Re: [PATCH 1/5] add SWAP macro

From: Jeff King <peff@peff.net>
Date: Jan 31, 2017, 22:36 UTC
Message-ID: <20170131223604.oa4h5ykhefvlergd@sigill.intra.peff.net>
In-Reply-To: <xmqqy3xqq2w0.fsf@gitster.mtv.corp.google.com>

On Tue, Jan 31, 2017 at 02:29:51PM -0800, Junio C Hamano wrote:

Show 26 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > ... I wonder if it would be more natural for it to take
> > pointers-to-objects, making it look more like a real function (i.e.,
> > SWAP(&a, &b) instead of SWAP(a, b)". And then these funny corner cases
> > become quite obvious in the caller, because the caller is the one who
> > has to type "&".
> 
> Hmmmm.  
> 
> While this looks very attractive in theory by forcing 'a' and 'b' to
> be lvalues, it probably invites mistakes go unnoticed during the
> review when the code wants to swap two pointer variables.  
> 
> For example,
> 
> apply.c:            SWAP(p->new_name, p->old_name);
> 
> is now a bug and will swap only the first byte of these names, and
> the correct way to spell it would become:
> 
> apply.c:            SWAP(&p->new_name, &p->old_name);
> 
> The latter clearly looks like swapping the new and old names, which
> is good, but I do not have any confidence that I will immediately
> spot a bug when presented the former under the new world order.

Yes, it's a problem with any function (or function-like interface) that takes an untyped pointer. You don't know if the caller meant the pointer or the pointer-to-pointer. In that sense it's no different than:

  memcpy(p->new_name, p->old_name, len);

Is that right, or did we mean to copy the pointers themselves?

It does also help the reverse case:

  int a, b;
  SWAP(a, b);

which would give an error (you forgot the "&"). I don't know which is more likely to be productive. But at least requiring pointer values makes it consistent with other functions like memcpy, etc.

-Peff