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

2 messages, 2 authors, 2019-09-30 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2019-09-30 01:36:47

Alex Henrie [off-list ref] 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.
quoted hunk
Signed-off-by: Alex Henrie <redacted>
---
 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;

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

From: Alex Henrie <hidden>
Date: 2019-09-30 07:45:28

On Sun, Sep 29, 2019 at 7:36 PM Junio C Hamano [off-list ref] wrote:
Alex Henrie [off-list ref] writes:
quoted
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help