Re: [PATCH 1/5] add SWAP macro

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 1, 2017, 18:33 UTC
Message-ID: <xmqqd1f1pxqc.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <bfd0d758-a9c8-9792-6294-9f9ed632cc98@web.de>

René Scharfe <l.s.r@web.de> writes:

> Size checks could be added to SIMPLE_SWAP as well.

Between SWAP() and SIMPLE_SWAP() I am undecided.

If the compiler can infer the type and the size of the two "locations" given to the macro, there is no technical reason to require the caller to specify the type as an extra argument, so SIMPLE_SWAP() may not necessarily an improvement over SWAP() from that point of view. If the redundancy is used as a sanity check, I'd be in favor of SIMPLE_SWAP(), though.

If the goal of SIMPLE_SWAP(), on the other hand, were to support the "only swap char with int for small value" example earlier in the thread, it means you cannot sanity check the type of things being swapped in the macro, and the readers of the code need to know about the details of what variables are being swapped. It looks to me that it defeats the main benefit of using a macro.

> The main benefit of a swap macro is reduced repetition IMHO: Users
> specify the variables to swap once instead of twice in a special
> order, and with SWAP they don't need to declare their type again.
> Squeezing out redundancy makes the code easier to read and modify.

Yes.