From: Junio C Hamano <gitster@pobox.com>
To: Alex Henrie <alexhenrie24@gmail.com>
Cc: Git mailing list <git@vger.kernel.org>,
CB Bailey <cb@hashpling.org>,
dstolee@microsoft.com
Subject: Re: [PATCH v4] diffcore-break: use a goto instead of a redundant if statement
Date: Wed, 02 Oct 2019 14:55:43 +0900 [thread overview]
Message-ID: <xmqqk19ng080.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <CAMMLpeQps8jvgpBXDnzqeeSsH7RA=__XjnDb4p_4SdGB-G2mSw@mail.gmail.com> (Alex Henrie's message of "Mon, 30 Sep 2019 11:36:46 -0600")
Alex Henrie <alexhenrie24@gmail.com> writes:
> 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.
> 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.
prev parent reply other threads:[~2019-10-02 5:55 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-29 20:43 [PATCH v4] diffcore-break: use a goto instead of a redundant if statement Alex Henrie
2019-09-30 1:36 ` Junio C Hamano
2019-09-30 7:45 ` Alex Henrie
2019-09-30 9:48 ` Junio C Hamano
2019-09-30 17:36 ` Alex Henrie
2019-10-02 5:55 ` Junio C Hamano [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqk19ng080.fsf@gitster-ct.c.googlers.com \
--to=gitster@pobox.com \
--cc=alexhenrie24@gmail.com \
--cc=cb@hashpling.org \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).