Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2014, 22:06 UTC
Message-ID: <xmqqmwi67cty.fsf@gitster.dls.corp.google.com>
In-Reply-To: <87zjm6v99y.fsf@fencepost.gnu.org>

David Kastrup <dak@gnu.org> writes:

Show 12 quoted lines

>> so something like
>>
>> 	for (p = buf; p < end; p++) {
>>         	p = find the end of this line
>>                 if (!p)
>>                 	break;
>> 		num++;
>> 	}
>>
>> perhaps?
>
> Would crash on incomplete last line.

Hmph, even with "if !p"?   From your other message:

+	for (p = buf;; num++) {
+		p = memchr(p, '\n', end - p);
+		if (p) {
+			p++;
+			continue;
 		}
+		break;
+	}

If you flip the if statement around that would be the same as:

+	for (p = buf;; num++) {
+		p = memchr(p, '\n', end - p);
+		if (!p)
			break;
		p++;
+	}

And with "loop action not in the control part", that would be the same as:

	for (p = buf; ; p++) {
		p = memchr...
                if (!p)
                	break;
		num++;
	}

no?  Would this crash on incomplete last line?

Ahh, "p < end" in "for (p = buf; p < end; p++) {" is not correct; you depend on overrunning the end of the buffer to feed length 0 to memchr and returning NULL inside the loop. So to be equivalent to your version, the contination condition needs to be

	for (p = buf; p <= end; p++) {

But that last round of the loop will be no-op, so "p < end" vs "p <= end" does not make any difference. It is not even strictly necessary because memchr() limits the scan to end, but it would still be a good belt-and-suspenders defensive coding practice, I would think.

+	for (p = buf;;) {
+		*lineno++ = p - buf;
+		p = memchr(p, '\n', end - p);
+		if (p) {
+			p++;
+			continue;
 		}
+		break;
 	}

Can we do the same transformation here?

	for (p = buf;;) {
        	*lineno++ = p - buf;
                p = memchr...
                if (!p)
                	break;
		p++;
	}

which is the same as

	for (p = buf; ; p++) {
        	*lineno++ = p - buf;
                p = memchr...
                if (!p)
                	break;
	}

and the latter has the loop control p++ at where it belongs to. The post-condition of one iteration of the body of the loop being "p points at the terminating LF of this line", incrementing the pointer is how the loop control advances the pointer to the beginning of the next line.

If we wanted to have a belt-and-suspenders safety, we need "p <= end" here, not "p < end", because of how p is used when it is past the last LF. As we want to make these two loops look similar, that means we should use "p <= end" in the previous loop as well.

This was fun ;-)