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

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

Hi Hannes,

On Fri, 4 Oct 2019, Johannes Sixt wrote:

> Am 04.10.19 um 11:55 schrieb Johannes Schindelin:
> > On Fri, 4 Oct 2019, Junio C Hamano wrote:
> >> These three look good and too similar to each other, which makes me
> >> wonder if we want to allow them simply write
> >>
> >> 	return insert_pos_as_negative_offset(nr);
> >>
> >> with something like
> >>
> >> 	static int insert_pos_as_negative_offset(uintmax_t nr)
> >> 	{
> >> 		if (INT_MAX < nr)
> >> 			die("overflow: -1 - %"PRIuMAX, nr);
> >> 		return -1 - (int)nr;
> >> 	}
> >>
> >> to avoid repetition.
> >
> > I tried not to do that because there are two different data types in
> > play: `unsigned int` and `size_t`. But I guess by making this an
> > `inline` function, compilers can optimize for the common case and avoid
> > casting _twice_.
> >
> > Will be fixed in v2,
>
> 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.

> The *real* problem to solve is to ensure that the index/cache does not
> grow so large that 32-bit indexes would be needed. Then the calculation
> that you want to hide here cannot overflow.

Well, that may be the real problem of another patch series. This patch
series' problem is to add a job to our Azure Pipeline that builds Git
with Visual Studio, and it patches the code minimally so that it builds
even in `DEVELOPER=1` mode, for good measure.

So I'd like not to dilute the purpose of this patch series.

Thanks,
Dscho