Re: [PATCH 1/5] add SWAP macro

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 1390 bytes --]

Hi Peff,

On Tue, 31 Jan 2017, Jeff King wrote:

> On Tue, Jan 31, 2017 at 10:03:01PM +0100, René Scharfe wrote:
> 
> > > Perhaps we could disallow a side-effect operator in the macro.  By
> > > disallow I mean place a comment at the definition to the macro and
> > > hopefully catch something like that in code-review.  We have the
> > > same issue with the `ALLOC_GROW()` macro.
> > 
> > SWAP(a++, ...) is caught by the compiler, SWAP(*a++, ...) works fine.
> > Technically that should be enough. :)  A comment wouldn't hurt, of
> > course.
> 
> One funny thing is that your macro takes address-of itself, behind the
> scenes. 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 "&".

But forcing SWAP(&a, &b) would make it even more cumbersome to use, and it
would also make it harder to optimize, say, by using registers instead of
addressable memory (think swapping two 32-bit integers: there is *no* need
to write them into memory just to swap them).

And I think I should repeat my point that this discussion veers towards
making simple swaps *more* complicated, rather than less complicated. Bad
direction.

Ciao,
Dscho