On Wed, Jul 14 2021, Taylor Blau wrote:
> On Fri, Jun 25, 2021 at 01:23:40AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Mon, Jun 21 2021, Taylor Blau wrote:
>>
>> > -static uint32_t find_object_pos(const struct object_id *oid)
>> > +static uint32_t find_object_pos(const struct object_id *oid, int *found)
>> > {
>> > struct object_entry *entry = packlist_find(writer.to_pack, oid);
>> >
>> > if (!entry) {
>> > - die("Failed to write bitmap index. Packfile doesn't have full closure "
>> > + if (found)
>> > + *found = 0;
>> > + warning("Failed to write bitmap index. Packfile doesn't have full closure "
>> > "(object %s is missing)", oid_to_hex(oid));
>> > + return 0;
>> > }
>> >
>> > + if (found)
>> > + *found = 1;
>> > return oe_in_pack_pos(writer.to_pack, entry);
>> > }
>>
>> So, a function that returns an unsigned 32 bit int won't (presumably)
>> have enough space for an "is bad", but before it died so it didn't
>> matter.
>>
>> Now it warns, so it needs a "is bad", so we add another "int" to pass
>> that information around.
>
> Right. You could imagine using the most-significant bit to indicate
> "bad" (which in this case is "I couldn't find this object that I'm
> supposed to be able to reach"), but of course it cuts our maximum number
> of objects in a bitmap in half.
>
>> 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 don't feel too strongly about it, though, so if you do I'd be happy to
> hear more.
I don't really care, it just looked a bit weird at first, and I wondered
why it couldn't return -1.
Aside from this case do you mean that such a cast would be too expensive
in general, or fears abou going past the 32 bits? I assumed that there
would be checks here for that already (and if not, we'd have wrap-around
now...).
