Re: [PATCH v5 10/13] xdiff: delete rchg aliasing

Ezekiel Newren <ezekielnewren@gmail.com> writes:

In general, I do not view the first one is a good excuse.

When using a separate local variable enhannces readability of the code (which often is true, with a pointer that points deep into a nested structure member) to humans, we shouldn't blindly bend the code to cater to less intelligent tools; it needs balancing.

The third one alone is not a good excuse for the same reason. It (and the first one) depends on how much benefit we are gaining from having a short-and-sweet local variables that may make the expressions and statements they are involved in easier to read.

For this particular change, I would think it is on borderline, and subjective. I would be OK with the third point if you rephrase it to additionally say that the conditional and the inner loop is easy enough to follow without using the local aliases to make the code shorter (which of course is the commit author's opinion, but they deserve to have and express their opinion as part of the rationale for a change).

Thanks.

-	char *rchg1 = xe->xdf1.rchg, *rchg2 = xe->xdf2.rchg;
 	long i1, i2, l1, l2;
 
 	/*
 	 * Trivial. Collects "groups" of changes and creates an edit script.
 	 */
 	for (i1 = xe->xdf1.nrec, i2 = xe->xdf2.nrec; i1 >= 0 || i2 >= 0; i1--, i2--)
-		if (rchg1[i1 - 1] || rchg2[i2 - 1]) {
-			for (l1 = i1; rchg1[i1 - 1]; i1--);
-			for (l2 = i2; rchg2[i2 - 1]; i2--);
+		if (xe->xdf1.rchg[i1 - 1] || xe->xdf2.rchg[i2 - 1]) {
+			for (l1 = i1; xe->xdf1.rchg[i1 - 1]; i1--);
+			for (l2 = i2; xe->xdf2.rchg[i2 - 1]; i2--);