Hi Hannes,
On Sun, 6 Oct 2019, Johannes Sixt wrote:
> Am 06.10.19 um 02:02 schrieb Junio C Hamano:
> > Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
> >
> >>> IMHO, if you don't accompany insert_pos_as_negative_offset() with a
> >>> corresponding extract_pos_and_found_condition() and use it everywhere,
> >>> it is more obfuscating than necessary.
> >>
> >> I do disagree here. No overflow checking needs to be performed for `-1 -
> >> <int-value>`. And that's what the opposite of this function really boils
> >> down to.
> >
> > I do not think j6t is referring to the over/underflow issues at all.
> >
> > The suggestion is that, because insert-pos-as-negative-offset
> > abstracts away [...] the fact that
> > "does not exist but here is the location it would be inserted" is
> > encoded in a certain way [...], the side that
> > consumes the encoded "pos" [...] should be abstracted away [...].
>
> Thank you, that summarizes my thoughts very well.
>
> > I think that is a reasonable thing to consider; it is not necessary
> > for correctness, but contributes to the conceptual clarity (iow, it
> > can be left as a separate clean-up step done after the series is
> > done).
>
> Thanks, but I disagree with the course of action: I think we should do
> both sides at the same time. (And if we aren't ready to do the decoding
> side now, then we should delay the encoding side).
>
> Consider someone is looking at the call site without knowing the
> detailed meaning of the return value ("What the heck is this -1
> business?"), it is a matter to look at the function being called to know
> what it is. With another function that does the encoding, it is one
> additional hop. That is my reason for saying "more obfuscating than
> necessary".
>
> That the helper function would reduce some small code duplication does
> not outweigh the obfuscation in my book. That's just MHO, of course.
So you got what you wished for:
https://public-inbox.org/git/pull.378.git.gitgitgadget@xxxxxxxxx
Care to review?
Thanks,
Dscho
