Re: [PATCH 1/1] Add a helper to reverse index_pos_to_insert_pos()

[Johannes Sixt <j6t@kdbg.org>]

Am 09.10.19 um 03:19 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> We do not want to have it for *all* cases, where we return -1 - pos, but
>> only for those cases, where the result was actually encoded by
>> index_pos_to_insert_pos().
> 
> Yup, I agree with you that decoder should be fed only the data
> emitted by the encoder.
> 
> But shouldn't the code that yielded 'pos' that later gets decoded by
> computing "-1 -pos" without using the encoding helper be corrected
> to use the encoder instead?

That is the obvious conclusion, of course.

>  After all, the primary purpose of
> inventing the encoder was to catch the arith overflow, wasn't it?

That was *your* motivation for the helper function. But IMO it is a
wrong design decision. Whether or not the index calculation overflows is
a matter of the type that is used for the index, and that in turn is
dicated by the possible sizes of the collections that are indexed. IOW,
the index overflow check is (*if* it is necessary) a policy decision
that must be made at a higher level and must not be hidden away in a
helper function whose purpose (as suggested by its name) is something
entirely different.

Unless, of course, we declare "all our indexes are of type int". But
that ship has sailed long ago, because there are too many cases where we
are forced to use size_t as index (strlen, sizeof...).

Meta note: We know that we are painting a tiny shed here (Replacing a
one-liner by a one-liner, huh?) If anyone of you has better things to
do, please move on. My interest in this discussion are just the design
decisions that are made, not the actual outcome of this particular case.

-- Hannes