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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 4, 2019, 21:24 UTC
Message-ID: <nycvar.QRO.7.76.6.1910042319190.46@tvgsbejvaqbjf.bet>
In-Reply-To: <ae6a64f4-8f46-cbaf-5004-defc316c5157@kdbg.org>

Hi Hannes,

On Fri, 4 Oct 2019, Johannes Sixt wrote:

Show 28 quoted lines

> 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