Re: [PATCH 1/5] add SWAP macro

From: Jeff King <peff@peff.net>
Date: Feb 1, 2017, 11:47 UTC
Message-ID: <20170201114750.r5xdy6emdczmnh4j@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.20.1702011225250.3469@virtualbox>

On Wed, Feb 01, 2017 at 12:28:13PM +0100, Johannes Schindelin wrote:

Show 11 quoted lines

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

I don't find the register thing compelling at all. I'd expect modern compilers to optimize *(&a) into just "a" and keep using a register. I'm sure there are compilers that don't, but I'm also not sure how much we _care_. If your compiler doesn't do basic micro-optimizations, then you live with it or get a better compiler.

I'm much more interested in what's readable and maintainable, versus trying to micro-optimize something that hasn't even been shown to be measurably interesting.

That said...

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

I'm not altogether convinced that SWAP() is an improvement in readability. I really like that it's shorter than the code it replaces, but there is a danger with introducing opaque constructs. It's one more thing for somebody familiar with C but new to the project to learn or get wrong.

IMHO the main maintenance gain from René's patch is that you don't have to specify the type, which means you can never have a memory-overflow bug due to incorrect types. If we lose that, then I don't see all that much value in the whole thing.

-Peff