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

From: Alex Henrie <alexhenrie24@gmail.com>
Date: Sep 30, 2019, 07:45 UTC
Message-ID: <CAMMLpeQLXN=jvD6MSJPdUTr60MiKdQq=zHFQ7aiatFuhqX1aeQ@mail.gmail.com>
In-Reply-To: <xmqqa7amimzc.fsf@gitster-ct.c.googlers.com>

On Sun, Sep 29, 2019 at 7:36 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > The condition "if (q->nr <= j)" checks whether the loop exited normally
> > or via a break statement. This check can be avoided by replacing the
> > jump out of the inner loop with a jump to the end of the outer loop.
> >
> > With the break replaced by a goto, the two diff_q calls then can be
> > replaced with a single diff_q call outside of the outer if statement.
>
> I doubt that it is a good idea to do these two things.  Especially I
> do not see why the latter makes the resulting code better.

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", 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