| Message ID | 20190929204322.1244907-1-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] diffcore-break: use a goto instead of a redundant if statement | expand |
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;
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
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
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
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.
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;
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(-)