Re: [PATCH v2 02/13] msvc: avoid using minus operator on unsigned types

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux