Re: [PATCH v2 11/18] git-compat-util.h: introduce `u32_add()`

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

> A future commit will want to add two 32-bit unsigned values together
> while checking for overflow. Introduce a variant of the u64_add()
> function for operating on 32-bit inputs.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  git-compat-util.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index b0673d1a450..db62a6f25c5 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -641,6 +641,14 @@ static inline int cast_size_t_to_int(size_t a)
>  	return (int)a;
>  }
>  
> +static inline uint32_t u32_add(uint32_t a, uint32_t b)
> +{
> +	if (unsigned_add_overflows(a, b))
> +		die("uint32_t overflow: %"PRIuMAX" + %"PRIuMAX,
> +		    (uintmax_t)a, (uintmax_t)b);
> +	return a + b;
> +}

Neither this one, nor the original u64_add(), seem to me a
particularly good API.

When things might overflow, it is a given that we should give a
controlled death rather than nonsense behaviour and/or corrupt
output, but shouldn't the diagnosis message given to the end-user
when we find an overflow be given at a bit higher layer?  At this
level, you do not even know what quantities you are adding together
*means*.  Even though your caller might be able to give a more
intelligible message like "the number of packs to combine exceeds
2^32 that is too many".  But dying in these functions means the
callers have no chance to do so.  The only thing the user sees is
some code tried to add two u32 and overflowed---without any hint
what these quantities were or what the addition was trying to
compute.