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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Hannes,

On Wed, 9 Oct 2019, Johannes Sixt wrote:

> Am 09.10.19 um 03:19 schrieb Junio C Hamano:
> > Johannes Sixt <j6t@kdbg.org> writes:
>
> >  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.

I wish that comment, and the argument following it, would have come as
part of the review of the patch series that already made it to `next`.

FWIW I actually agree with Junio about the helper, but in hindsight I
could have used a better name (not one that is tied to the "index").
Something like `unsigned_one_complement()`. But of course, that would
say _what_ it does, not _why_.

And yes, I would wish we had C++ templates so that the helper could use
the exact type of the caller.

Ciao,
Dscho

> 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
>