On 1/14/2021 1:46 AM, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> writes:
>
>> +/*
>> + * offset_to_pack_pos converts an object offset to a pack position. This
>> + * function returns zero on success, and a negative number otherwise. The
>> + * parameter 'pos' is usable only on success.
>> + *
>> + * If the reverse index has not yet been loaded, this function loads it lazily,
>> + * and returns an negative number if an error was encountered.
>
> It is somewhat strange to see a function that yields a non-negative
> "position" on success and a negative value to signal a failure to
> have a separate pointer to the location to receive the true return
> value. Do we truly care the upper half of "uint32_t" (in other
> words, do we seriously want to support more than 2G positions in a
> pack)?
>
> What I'm trying to get at is that
>
> int pos = offset_to_pack_pos(...);
> if (pos < 0)
> error();
> else
> use(pos);
>
> is more natural than
I agree that this is used commonly, but usually in the case that
we are finding a position in the list _or where such an item would
be inserted_. For example:
pos = index_name_pos(istate, dirname, len);
if (pos < 0)
pos = -pos-1;
while (pos < istate->cache_nr) {
...
But that does not apply in this case. Knowing that the requested
offset lies between object 'i' and object 'i + 1' isn't helpful,
since the offset still does not correspond to the start of an
object.
> uint32_t pos;
> if (offset_to_pack_pos(..., &pos) < 0)
> error();
> else
> use(pos);
>
> but now I wrote it down and laid it out in front of my eyes, the
> latter does not look too bad.
>
> ... later comes back after reading through the series ...
>
> The new callers all looked quite nice to eyes. Because we
> discourage assignment inside if() condition, the converted
> result does not make the code more verbose than the
> original. In fact, it makes it even clearer that we are
> checking for an error return from a function call.
>
> Quite nice.
As someone who spends a decent amount of time working in C#, I
also like this pattern. The APIs in C# work this way, too, such
as:
if (!set.TryGetValue(key, out value))
return false;
// Use 'value', which is initialized now.
Thanks,
-Stolee
