{"thread":{"id":"51941","subject":"[PATCH v4] diffcore-break: use a goto instead of a redundant if statement","startedAt":"2019-09-29T20:51:24Z","lastAt":"2019-10-02T05:55:52Z","messageCount":6,"participants":["Alex Henrie","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"383107","messageId":"20190929204322.1244907-1-alexhenrie24@gmail.com","threadId":"51941","inReplyTo":null,"subject":"[PATCH v4] diffcore-break: use a goto instead of a redundant if statement","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-09-29T20:43:22Z","receivedAt":"2019-09-29T20:51:24Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"The condition \"if (q->nr <= j)\" checks whether the loop exited normally\nor via a break statement. This check can be avoided by replacing the\njump out of the inner loop with a jump to the end of the outer loop.\n\nWith the break replaced by a goto, the two diff_q calls then can be\nreplaced with a single diff_q call outside of the outer if statement.\n\nSigned-off-by: Alex Henrie \n---\n diffcore-break.c | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/diffcore-break.c b/diffcore-break.c\nindex 875aefd3fe..ee7519d959 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -286,17 +286,15 @@ void diffcore_merge_broken(void)\n \t\t\t\t\t/* Peer survived. Merge them */\n \t\t\t\t\tmerge_broken(p, pp, &outq);\n \t\t\t\t\tq->queue[j] = NULL;\n-\t\t\t\t\tbreak;\n+\t\t\t\t\tgoto next;\n \t\t\t\t}\n \t\t\t}\n-\t\t\tif (q->nr <= j)\n-\t\t\t\t/* The peer did not survive, so we keep\n-\t\t\t\t * it in the output.\n-\t\t\t\t */\n-\t\t\t\tdiff_q(&outq, p);\n+\t\t\t/* The peer did not survive, so we keep\n+\t\t\t * it in the output.\n+\t\t\t */\n \t\t}\n-\t\telse\n-\t\t\tdiff_q(&outq, p);\n+\t\tdiff_q(&outq, p);\n+next:;\n \t}\n \tfree(q->queue);\n \t*q = outq;\n-- \n2.23.0\n\n"},{"id":"383111","messageId":"xmqqa7amimzc.fsf@gitster-ct.c.googlers.com","threadId":"51941","inReplyTo":"20190929204322.1244907-1-alexhenrie24@gmail.com","subject":"Re: [PATCH v4] diffcore-break: use a goto instead of a redundant if statement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-30T01:36:39Z","receivedAt":"2019-09-30T01:36:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n> The condition \"if (q->nr <= j)\" checks whether the loop exited normally\n> or via a break statement. This check can be avoided by replacing the\n> jump out of the inner loop with a jump to the end of the outer loop.\n>\n> With the break replaced by a goto, the two diff_q calls then can be\n> replaced with a single diff_q call outside of the outer if statement.\n\nI doubt that it is a good idea to do these two things. Especially I\ndo not see why the latter makes the resulting code better.\n\n> Signed-off-by: Alex Henrie \n> ---\n> diffcore-break.c | 14 ++++++--------\n> 1 file changed, 6 insertions(+), 8 deletions(-)\n>\n> diff --git a/diffcore-break.c b/diffcore-break.c\n> index 875aefd3fe..ee7519d959 100644\n> --- a/diffcore-break.c\n> +++ b/diffcore-break.c\n> @@ -286,17 +286,15 @@ void diffcore_merge_broken(void)\n> \t\t\t\t\t/* Peer survived. Merge them */\n> \t\t\t\t\tmerge_broken(p, pp, &outq);\n> \t\t\t\t\tq->queue[j] = NULL;\n> -\t\t\t\t\tbreak;\n> +\t\t\t\t\tgoto next;\n> \t\t\t\t}\n> \t\t\t}\n> -\t\t\tif (q->nr <= j)\n> -\t\t\t\t/* The peer did not survive, so we keep\n> -\t\t\t\t * it in the output.\n> -\t\t\t\t */\n> -\t\t\t\tdiff_q(&outq, p);\n> +\t\t\t/* The peer did not survive, so we keep\n> +\t\t\t * it in the output.\n> +\t\t\t */\n> \t\t}\n> -\t\telse\n> -\t\t\tdiff_q(&outq, p);\n> +\t\tdiff_q(&outq, p);\n> +next:;\n> \t}\n> \tfree(q->queue);\n> \t*q = outq;\n"},{"id":"383128","messageId":"CAMMLpeQLXN=jvD6MSJPdUTr60MiKdQq=zHFQ7aiatFuhqX1aeQ@mail.gmail.com","threadId":"51941","inReplyTo":"xmqqa7amimzc.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4] diffcore-break: use a goto instead of a redundant if statement","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-09-30T07:45:12Z","receivedAt":"2019-09-30T07:45:28Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Sun, Sep 29, 2019 at 7:36 PM Junio C Hamano wrote:\n>\n> Alex Henrie writes:\n>\n> > The condition \"if (q->nr <= j)\" checks whether the loop exited normally\n> > or via a break statement. This check can be avoided by replacing the\n> > jump out of the inner loop with a jump to the end of the outer loop.\n> >\n> > With the break replaced by a goto, the two diff_q calls then can be\n> > replaced with a single diff_q call outside of the outer if statement.\n>\n> I doubt that it is a good idea to do these two things. Especially I\n> do not see why the latter makes the resulting code better.\n\nWell, I admit that code clarity is somewhat subjective. To me it's not\nobvious that \"if (q->nr <= j)\" means \"if the loop exited normally\",\nbut a goto does make it obvious. (And it's definitely more clear to\nscan-build, which complains about a possible memory leak when an if\nstatement is used but does not complain when the if statement is\nreplaced with a goto.)\n\nAs far as the diff_q calls, I think that having one call instead of\ntwo is slightly more readable, but I don't care very much about it.\nI'd be happy to drop that change from the next version of the patch.\n\n-Alex\n"},{"id":"383134","messageId":"xmqq36gei07z.fsf@gitster-ct.c.googlers.com","threadId":"51941","inReplyTo":"CAMMLpeQLXN=jvD6MSJPdUTr60MiKdQq=zHFQ7aiatFuhqX1aeQ@mail.gmail.com","subject":"Re: [PATCH v4] diffcore-break: use a goto instead of a redundant if statement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-30T09:48:16Z","receivedAt":"2019-09-30T09:48:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n> Well, I admit that code clarity is somewhat subjective. To me it's not\n> obvious that \"if (q->nr <= j)\" means \"if the loop exited normally\",\n\nI actually do not have too much problem with this side of the\nequation. I however do see problem with squashing the two diff_q\ncalls that _happens_ to be textually the same but is made from two\nlogically separate codepaths (cases B.1 and C, in the message you\nare responding to but omitted from quote). After all, you did not\neven realize you introduced a new bug by doing so before CB pointed\nit out, no? A rewrite like that that easily allows a new bug to\nslip in hardly qualifies as making code \"more clear and readable\".\n\n> but a goto does make it obvious. (And it's definitely more clear to\n> scan-build, which complains about a possible memory leak when an if\n> statement is used but does not complain when the if statement is\n> replaced with a goto.)\n>\n> As far as the diff_q calls, I think that having one call instead of\n> two is slightly more readable, but I don't care very much about it.\n> I'd be happy to drop that change from the next version of the patch.\n>\n> -Alex\n"},{"id":"383179","messageId":"CAMMLpeQps8jvgpBXDnzqeeSsH7RA=__XjnDb4p_4SdGB-G2mSw@mail.gmail.com","threadId":"51941","inReplyTo":"xmqq36gei07z.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4] diffcore-break: use a goto instead of a redundant if statement","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-09-30T17:36:46Z","receivedAt":"2019-09-30T21:23:54Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Mon, Sep 30, 2019 at 3:48 AM Junio C Hamano wrote:\n>\n> Alex Henrie writes:\n>\n> > Well, I admit that code clarity is somewhat subjective. To me it's not\n> > obvious that \"if (q->nr <= j)\" means \"if the loop exited normally\",\n>\n> I actually do not have too much problem with this side of the\n> equation. I however do see problem with squashing the two diff_q\n> calls that _happens_ to be textually the same but is made from two\n> logically separate codepaths (cases B.1 and C, in the message you\n> are responding to but omitted from quote). After all, you did not\n> even realize you introduced a new bug by doing so before CB pointed\n> it out, no? A rewrite like that that easily allows a new bug to\n> slip in hardly qualifies as making code \"more clear and readable\".\n\nSure, let's keep the two diff_q calls. I'll send a new patch.\n\nTo me at least it was not clear what code is executed if the peer\nsurvives. The fact that I put the goto label in the wrong place the\nfirst time only underscores the difficulty of understanding this\nfunction. But with a goto (and with its label in the correct place),\nthe execution path is obvious.\n\nThank you for being willing to look at this code with me, and thank\nyou for your feedback!\n\n-Alex\n"},{"id":"383246","messageId":"xmqqk19ng080.fsf@gitster-ct.c.googlers.com","threadId":"51941","inReplyTo":"CAMMLpeQps8jvgpBXDnzqeeSsH7RA=__XjnDb4p_4SdGB-G2mSw@mail.gmail.com","subject":"Re: [PATCH v4] diffcore-break: use a goto instead of a redundant if statement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-02T05:55:43Z","receivedAt":"2019-10-02T05:55:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n> On Mon, Sep 30, 2019 at 3:48 AM Junio C Hamano wrote:\n>>\n>> Alex Henrie writes:\n>>\n>> > Well, I admit that code clarity is somewhat subjective. To me it's not\n>> > obvious that \"if (q->nr <= j)\" means \"if the loop exited normally\",\n\nI agree that it is subjective, but \"if counter hasn't run beyond the\nlimit\" immediately after a loop is a pretty-well established idiom\nto see if we broke out prematurely.\n\n> To me at least it was not clear what code is executed if the peer\n> survives. The fact that I put the goto label in the wrong place the\n> first time only underscores the difficulty of understanding this\n> function. But with a goto (and with its label in the correct place),\n> the execution path is obvious.\n\nWell, the fact that the version with goto jumping to a wrong place\nwas so easy to spot as buggy might be an indication that goto made\nit obvious to CB, but clearly use of goto did not make logic obvious\nat least to the older version of you who wrote the buggy code.\n\nWell I am being a bit mean in the above ;-) A more fair statement\nwould be that your bug did not come from goto; I think what\ncontributed more to the logic flaw in your earlier round was the\nmerging of two diff_q() calls from unrelated codepath. Without that\nchange, and with the knowledge of \"did the loop terminate early\"\nidiom, the version without 'goto' is obvious, and with only 'goto'\nchange, that original obviousness would not diminish.\n"}]}