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

From: Denton Liu <liu.denton@gmail.com>
Date: Sep 26, 2019, 17:20 UTC
Message-ID: <20190926172022.GA41037@generichostname>
In-Reply-To: <2abe1e1fb0bf3025489c2e543b9a9c648a164827.1569486607.git.gitgitgadget@gmail.com>

Hi Dscho,

On Thu, Sep 26, 2019 at 01:30:10AM -0700, Johannes Schindelin via GitGitGadget wrote:

Show 10 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> MSVC complains about this with `-Wall`, which can be taken as a sign
> that this is indeed a real bug. The symptom is:
> 
> 	C4146: unary minus operator applied to unsigned type, result
> 	still unsigned
> 
> Let's avoid this warning in the minimal way, e.g. writing `-1 -
> <unsigned value>` instead of `-<unsigned value> - 1`.

[...]

Show 15 quoted lines

> ---
>  read-cache.c  | 4 ++--
>  sha1-lookup.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index c701f7f8b8..11f3357216 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1276,7 +1276,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
>  	 */
>  	if (istate->cache_nr > 0 &&
>  		strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0)
> -		pos = -istate->cache_nr - 1;
> +		pos = -1 - istate->cache_nr;

I've been thinking about this and I'm still not certain that this 100% correct from a language-lawyer perspective.

If we do `-1 - istate->cache_nr`, then the unsignedness of istate->cache_nr takes over and the whole expression is a very large unsigned number.

Then, when we assign to `int pos`, we are converting an unsigned number which is out of the range of the signed number. According to a StackOverflow post citing the C99 standard[1]:

	Otherwise, the new type is signed and the value cannot be
	represented in it; either the result is implementation-defined
	or an implementation-defined signal is raised.

I'm sure that most platforms that we support will handle it sanely but could we write this as

	pos = -1 - (int) istate->cache_nr;

to be doubly sure that no funny business will happen?

Show 22 quoted lines

>  	else
>  		pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce));
>  
> @@ -1894,7 +1894,7 @@ static size_t estimate_cache_size(size_t ondisk_size, unsigned int entries)
>  	/*
>  	 * Account for potential alignment differences.
>  	 */
> -	per_entry += align_padding_size(sizeof(struct cache_entry), -sizeof(struct ondisk_cache_entry));
> +	per_entry += align_padding_size(per_entry, 0);
>  	return ondisk_size + entries * per_entry;
>  }
>  
> diff --git a/sha1-lookup.c b/sha1-lookup.c
> index 796ab68da8..c819687730 100644
> --- a/sha1-lookup.c
> +++ b/sha1-lookup.c
> @@ -97,7 +97,7 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t nr,
>  			lo = mi + 1;
>  		mi = lo + (hi - lo) / 2;
>  	} while (lo < hi);
> -	return -lo-1;
> +	return -1 - lo;

Same thing here.

[1]: https://stackoverflow.com/questions/50605/signed-to-unsigned-conversion-in-c-is-it-always-safe

Show 6 quoted lines

>  }
>  
>  int bsearch_hash(const unsigned char *sha1, const uint32_t *fanout_nbo,
> -- 
> gitgitgadget
>