Re: [PATCH v2 02/13] msvc: avoid using minus operator on unsigned types

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 6, 2019, 00:02 UTC
Message-ID: <xmqqr23q68rg.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.1910042319190.46@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 7 quoted lines

>> IMHO, if you don't accompany insert_pos_as_negative_offset() with a
>> corresponding extract_pos_and_found_condition() and use it everywhere,
>> it is more obfuscating than necessary.
>
> I do disagree here. No overflow checking needs to be performed for `-1 -
> <int-value>`. And that's what the opposite of this function really boils
> down to.

I do not think j6t is referring to the over/underflow issues at all.

The suggestion is that, because insert-pos-as-negative-offset abstracts away (in addition to the overflow checks) the fact that "does not exist but here is the location it would be inserted" is encoded in a certain way (i.e. not just the array index negated, but also is offset by -1, because we wouldn't be able to say "at the very beginning at index 0" without the -1 offset), the side that consumes the encoded "pos" (i.e. "we got a negative, so we know the element does not exist, and the index into the array we would insert a new element is computed this way") should be abstracted away, as it must know that the extra negative offset used when encoding is "-1".

I think that is a reasonable thing to consider; it is not necessary for correctness, but contributes to the conceptual clarity (iow, it can be left as a separate clean-up step done after the series is done).