Re: [PATCH 1/5] add SWAP macro

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

Jeff King [off-list ref] writes:

quoted

... 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