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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 30, 2019, 09:48 UTC
Message-ID: <xmqq36gei07z.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAMMLpeQLXN=jvD6MSJPdUTr60MiKdQq=zHFQ7aiatFuhqX1aeQ@mail.gmail.com>

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 actually do not have too much problem with this side of the equation. I however do see problem with squashing the two diff_q calls that _happens_ to be textually the same but is made from two logically separate codepaths (cases B.1 and C, in the message you are responding to but omitted from quote). After all, you did not even realize you introduced a new bug by doing so before CB pointed it out, no? A rewrite like that that easily allows a new bug to slip in hardly qualifies as making code "more clear and readable".

Show 10 quoted lines

> but a goto does make it obvious. (And it's definitely more clear to
> scan-build, which complains about a possible memory leak when an if
> statement is used but does not complain when the if statement is
> replaced with a goto.)
>
> As far as the diff_q calls, I think that having one call instead of
> two is slightly more readable, but I don't care very much about it.
> I'd be happy to drop that change from the next version of the patch.
>
> -Alex