Junio C Hamano <gits...@pobox.com> writes: > David Kastrup <d...@gnu.org> writes: > >> Junio C Hamano <gits...@pobox.com> writes: >> >>> which I think is the prevalent style in our codebase. The same for >>> the other loop we see in the new code below. >>> >>> - avoid assignments in conditionals when you do not have to. >> >> commit a77a48c259d9adbe7779ca69a3432e493116b3fd >> Author: Junio C Hamano <gits...@pobox.com> >> Date: Tue Jan 28 13:55:59 2014 -0800 >> >> combine-diff: simplify intersect_paths() further >> [...] >> >> + while ((p = *tail) != NULL) { >> >> Because we can. > > Be reasonable. You cannot sensibly rewrite it to > > p = *tail; > while (p) { > ... > p = *tail; > } > > when you do not know how ... part would evolve in the future.
The only unknown here is the potential presence of "continue;" in ... and that can be addressed by writing for (p = *tail; p; p = *tail) { ... } However, that only makes sense where ... is rather large and diverse and the assignment in question provides a unifying point. In this case, the loop is rather small and perfectly fits on one screen. It turns out that the assignment only serves for _obfuscating_ the various code paths. We have: while ((p = *tail) != NULL) { cmp = ((i >= q->nr) ? -1 : strcmp(p->path, q->queue[i]->two->path)); if (cmp < 0) { /* p->path not in q->queue[]; drop it */ *tail = p->next; free(p); continue; } if (cmp > 0) { /* q->queue[i] not in p->path; skip it */ i++; continue; } hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1); p->parent[n].mode = q->queue[i]->one->mode; p->parent[n].status = q->queue[i]->status; tail = &p->next; i++; } While we could instead have: p = curr; while (p) { cmp = ((i >= q->nr) ? -1 : strcmp(p->path, q->queue[i]->two->path)); if (cmp < 0) { struct combine_diff_path *n = p->next; /* p->path not in q->queue[]; drop it */ free(p); p = *tail = n; continue; } if (cmp > 0) { /* q->queue[i] not in p->path; skip it */ i++; continue; } hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1); p->parent[n].mode = q->queue[i]->one->mode; p->parent[n].status = q->queue[i]->status; p = *(tail = &p->next); i++; } Of course, it only makes limited sense to recheck p after the second if, so it would be clearer to write p = curr; while (p) { cmp = ((i >= q->nr) ? -1 : strcmp(p->path, q->queue[i]->two->path)); if (cmp < 0) { struct combine_diff_path *n = p->next; /* p->path not in q->queue[]; drop it */ free(p); p = *tail = n; continue; } if (cmp > 0) { /* q->queue[i] not in p->path; skip it */ i++; continue; } hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1); p->parent[n].mode = q->queue[i]->one->mode; p->parent[n].status = q->queue[i]->status; p = *(tail = &p->next); i++; } But that's sort of a red herring since the actual loop structure is hidden in conditions where it does not belong. (i >= q->nr) is a _terminal_ condition. So it's more like p = curr; while (p) { if (i >= q->nr) { *tail = NULL; do { struct combine_diff_path *n = p->next; free(p); p = n; } while (p); break; } cmp = strcmp(p->path, q->queue[i]->two->path)); if (cmp < 0) { struct combine_diff_path *n = p->next; /* p->path not in q->queue[]; drop it */ free(p); p = *tail = n; continue; } if (cmp == 0) { hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1); p->parent[n].mode = q->queue[i]->one->mode; p->parent[n].status = q->queue[i]->status; p = *(tail = &p->next); } i++; } > if ((p = *tail) != NULL) { > ... > > is a totally different issue. Yes: it was just a matter of style instead of preventing _other_ code to be rewritten in a clearer manner. For a "don't look elsewhere" solution, while ((p = *tail) != NULL) can _always_ be equivalently replaced with for (p = *tail; p; p = *tail) and in this case already trivially improved with for (p = curr; p; p = *tail) which meets your style prescription "avoid assignments in conditionals when you do not have to." but in this particular case, the "don't look elsewhere" solution was not called for. It unifies code paths that deserve to stay separate: we don't _want_ the assignment to take place for every path leading to the loop control. It makes it less clear to see what happens. -- David Kastrup -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html