* [PATCH v4] diffcore-break: use a goto instead of a redundant if statement
@ 2019-09-29 20:43 Alex Henrie
2019-09-30 1:36 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Alex Henrie @ 2019-09-29 20:43 UTC (permalink / raw)
To: git, cb, dstolee, gitster; +Cc: Alex Henrie
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.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
diffcore-break.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/diffcore-break.c b/diffcore-break.c
index 875aefd3fe..ee7519d959 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -286,17 +286,15 @@ void diffcore_merge_broken(void)
/* Peer survived. Merge them */
merge_broken(p, pp, &outq);
q->queue[j] = NULL;
- break;
+ goto next;
}
}
- if (q->nr <= j)
- /* The peer did not survive, so we keep
- * it in the output.
- */
- diff_q(&outq, p);
+ /* The peer did not survive, so we keep
+ * it in the output.
+ */
}
- else
- diff_q(&outq, p);
+ diff_q(&outq, p);
+next:;
}
free(q->queue);
*q = outq;
--
2.23.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] diffcore-break: use a goto instead of a redundant if statement
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
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2019-09-30 1:36 UTC (permalink / raw)
To: Alex Henrie; +Cc: git, cb, dstolee
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.
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> diffcore-break.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/diffcore-break.c b/diffcore-break.c
> index 875aefd3fe..ee7519d959 100644
> --- a/diffcore-break.c
> +++ b/diffcore-break.c
> @@ -286,17 +286,15 @@ void diffcore_merge_broken(void)
> /* Peer survived. Merge them */
> merge_broken(p, pp, &outq);
> q->queue[j] = NULL;
> - break;
> + goto next;
> }
> }
> - if (q->nr <= j)
> - /* The peer did not survive, so we keep
> - * it in the output.
> - */
> - diff_q(&outq, p);
> + /* The peer did not survive, so we keep
> + * it in the output.
> + */
> }
> - else
> - diff_q(&outq, p);
> + diff_q(&outq, p);
> +next:;
> }
> free(q->queue);
> *q = outq;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] diffcore-break: use a goto instead of a redundant if statement
2019-09-30 1:36 ` Junio C Hamano
@ 2019-09-30 7:45 ` Alex Henrie
2019-09-30 9:48 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Alex Henrie @ 2019-09-30 7:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git mailing list, CB Bailey, dstolee
On Sun, Sep 29, 2019 at 7:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] diffcore-break: use a goto instead of a redundant if statement
2019-09-30 7:45 ` Alex Henrie
@ 2019-09-30 9:48 ` Junio C Hamano
2019-09-30 17:36 ` Alex Henrie
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2019-09-30 9:48 UTC (permalink / raw)
To: Alex Henrie; +Cc: Git mailing list, CB Bailey, dstolee
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".
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] diffcore-break: use a goto instead of a redundant if statement
2019-09-30 9:48 ` Junio C Hamano
@ 2019-09-30 17:36 ` Alex Henrie
2019-10-02 5:55 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Alex Henrie @ 2019-09-30 17:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git mailing list, CB Bailey, dstolee
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 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".
Sure, let's keep the two diff_q calls. I'll send a new patch.
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.
Thank you for being willing to look at this code with me, and thank
you for your feedback!
-Alex
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] diffcore-break: use a goto instead of a redundant if statement
2019-09-30 17:36 ` Alex Henrie
@ 2019-10-02 5:55 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2019-10-02 5:55 UTC (permalink / raw)
To: Alex Henrie; +Cc: Git mailing list, CB Bailey, dstolee
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-02 5:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).