Re: [PATCH v4] diffcore-break: use a goto instead of a redundant if statement

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 2, 2019, 05:55 UTC
Message-ID: <xmqqk19ng080.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAMMLpeQps8jvgpBXDnzqeeSsH7RA=__XjnDb4p_4SdGB-G2mSw@mail.gmail.com>

Alex Henrie <alexhenrie24@gmail.com> writes:

Show 6 quoted lines

> On Mon, Sep 30, 2019 at 3:48 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Alex Henrie <alexhenrie24@gmail.com> writes:
>>
>> > Well, I admit that code clarity is somewhat subjective. To me it's not
>> > obvious that "if (q->nr <= j)" means "if the loop exited normally",

I agree that it is subjective, but "if counter hasn't run beyond the limit" immediately after a loop is a pretty-well established idiom to see if we broke out prematurely.

Show 5 quoted lines

> To me at least it was not clear what code is executed if the peer
> survives. The fact that I put the goto label in the wrong place the
> first time only underscores the difficulty of understanding this
> function. But with a goto (and with its label in the correct place),
> the execution path is obvious.

Well, the fact that the version with goto jumping to a wrong place was so easy to spot as buggy might be an indication that goto made it obvious to CB, but clearly use of goto did not make logic obvious at least to the older version of you who wrote the buggy code.

Well I am being a bit mean in the above ;-) A more fair statement would be that your bug did not come from goto; I think what contributed more to the logic flaw in your earlier round was the merging of two diff_q() calls from unrelated codepath. Without that change, and with the knowledge of "did the loop terminate early" idiom, the version without 'goto' is obvious, and with only 'goto' change, that original obviousness would not diminish.