From: David Kastrup <dak@gnu.org>
Date: Wed, 05 Feb 2014 23:45:50 GMT
Subject: Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line
Message-ID: <87vbwtjf81.fsf@fencepost.gnu.org>
In-Reply-To: <xmqqob2l2ta7.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> Junio C Hamano <gitster@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 <gitster@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