Re: [PATCH v2 11/18] git-compat-util.h: introduce `u32_add()`
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jan 21, 2026, 23:55 UTC
- Message-ID
- <aXFni2tE7vn1dKFp@nand.local>
- In-Reply-To
- <aXCTkVpjJkTabx_0@pks.im>
On Wed, Jan 21, 2026 at 09:51:36AM +0100, Patrick Steinhardt wrote:
Show 39 quoted lines
> > diff --git a/midx-write.c b/midx-write.c
> > index 87b97c70872..6006b6569c8 100644
> > --- a/midx-write.c
> > +++ b/midx-write.c
> > @@ -1738,8 +1738,19 @@ static void fill_included_packs_batch(struct repository *r,
> > */
> > expected_size = (uint64_t)pack_info[i].referenced_objects << 14;
> > expected_size /= p->num_objects;
> > - expected_size = u64_mult(expected_size, p->pack_size);
> > - expected_size = u64_add(expected_size, 1u << 13) >> 14;
> > +
> > + if (unsigned_mult_overflows(expected_size,
> > + (uint64_t)p->pack_size))
> > + die(_("overflow during fixed-point multiply (%"PRIu64" "
> > + "* %"PRIu64")"),
> > + expected_size, (uint64_t)p->pack_size);
> > + expected_size = expected_size * p->pack_size;
> > +
> > + if (unsigned_add_overflows(expected_size, 1u << 13))
> > + die(_("overflow during fixed-point rounding (%"PRIu64" "
> > + " + %"PRIu64")"),
> > + expected_size, (uint64_t)(1ul << 13));
> > + expected_size = (expected_size + (1u << 13)) >> 14;
>
> One downside this pattern has is that we repeat the computation, which
> makes it easy to get it wrong or forget updating either the check or the
> computation.
>
> I think ideally, we would have interfaces that combine the two
> approaches in `u64_mult()` and `unsigned_mult_overflows()`. Something
> like this for example:
>
> static intline bool u64_mult(uint64_t a, uint64_t b, uint64_t *out)
> {
> if (unsigned_mult_overflows(a, b))
> return false;
> *out = a * b;
> return true;
> }I had considered this approach when writing, but ultimately decided against it, since it felt a little clunky to have to pass a pointer in to do a simple arithmetic operation. But I think your point about ensuring that we actually do:
if (unsigned_mult_overflows(a, b))
die(...);
result = a * b;and not "result = a * c" or some other expression which is not "a * b" is a good one.
I dunno. The spots in this patch are the only uses of u64_mult() and u64_add(), so I'm hesitant to keep a helper function around just for that sole use-case. I wonder if we should do what you suggest here for the much more frequently used st_add() / st_mult() / st_sub() functions?
Show 6 quoted lines
> This would let the caller handle the failure and is thus quite flexible,
> which results in the following code:
>
> if (!u64_mult(expected_size, (uint64_t)p->pack_size, &expected_size))
> die(_("overflow during fixed-point multiply (%"PRIu64" "
> "* %"PRIu64")"), expected_size, (uint64_t)p->pack_size);It does read quite cleanly, so I think I'm convinced.
Thanks, Taylor