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

[Johannes Sixt <j6t@kdbg.org>]

Am 06.10.19 um 02:02 schrieb Junio C Hamano:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>>> 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 [...] the fact that
> "does not exist but here is the location it would be inserted" is
> encoded in a certain way [...], the side that
> consumes the encoded "pos" [...] should be abstracted away [...].

Thank you, that summarizes my thoughts very well.

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

Thanks, but I disagree with the course of action: I think we should do
both sides at the same time. (And if we aren't ready to do the decoding
side now, then we should delay the encoding side).

Consider someone is looking at the call site without knowing the
detailed meaning of the return value ("What the heck is this -1
business?"), it is a matter to look at the function being called to know
what it is. With another function that does the encoding, it is one
additional hop. That is my reason for saying "more obfuscating than
necessary".

That the helper function would reduce some small code duplication does
not outweigh the obfuscation in my book. That's just MHO, of course.

-- Hannes