Re: [PATCH v2 02/24] pack-bitmap-write.c: gracefully fail to write non-closed bitmaps

On Wed, Jul 14, 2021 at 01:32:21PM -0400, Taylor Blau wrote:

quoted

So if we're already paying for that extra space (which, on some
platforms would already be a 64 bit int, and on some so would the
uint32_t, it's just "at least 32 bits").

Wouldn't it be more idiomatic to just have find_object_pos() return
int64_t now, if it's -1 it's an error, otherwise the "pos" is cast to
uint32_t:

I'm not sure. It does save the extra argument, which is arguably more
convenient for callers, but the cost for doing so is a cast from a
signed integer type to an unsigned one (and a narrower destination type,
at that).

That seems easier to get wrong to me than passing a pointer to a pure
"int" and keeping the return type a uint32_t. So, I'm probably more
content to leave it as-is rather than change it.

I agree that the separate "found" value makes things more obvious.
Casting to a smaller size means explaining why that is OK, whereas it is
quite clear that things are correct with two separate variables. And
having an int64_t makes one wonder whether a value like 2^35 is possible
(it isn't; this is a uint32_t because that is the limit of objects in
the pack format).

-Peff