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

From: David Kastrup <dak@gnu.org>
Date: Feb 4, 2014, 21:27 UTC
Message-ID: <874n4ewouz.fsf@fencepost.gnu.org>
In-Reply-To: <xmqqd2j28w3h.fsf@gitster.dls.corp.google.com>

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

Show 44 quoted lines

> David Kastrup <dak@gnu.org> writes:
>
>> Making a single preparation run for counting the lines will avoid memory
>> fragmentation.  Also, fix the allocated memory size which was wrong
>> when sizeof(int *) != sizeof(int), and would have been too small
>> for sizeof(int *) < sizeof(int), admittedly unlikely.
>>
>> Signed-off-by: David Kastrup <dak@gnu.org>
>> ---
>>  builtin/blame.c | 40 ++++++++++++++++++++++++----------------
>>  1 file changed, 24 insertions(+), 16 deletions(-)
>>
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> index e44a6bb..522986d 100644
>> --- a/builtin/blame.c
>> +++ b/builtin/blame.c
>> @@ -1772,25 +1772,33 @@ static int prepare_lines(struct scoreboard *sb)
>>  {
>>  	const char *buf = sb->final_buf;
>>  	unsigned long len = sb->final_buf_size;
>> -	int num = 0, incomplete = 0, bol = 1;
>> +	const char *end = buf + len;
>> +	const char *p;
>> +	int *lineno;
>> +	
>> +	int num = 0, incomplete = 0;
>
> Is there any significance to the blank line between these two
> variable definitions?
>
>> +
>> +	for (p = buf;;) {
>> +		if ((p = memchr(p, '\n', end-p)) == NULL)
>> +			break;
>> +		++num, ++p;
>
> You have a peculiar style that is somewhat distracting.  Why isn't
> this more like so?
>
> 	for (p = buf; p++, num++; ) {
> 		p = memchr(p, '\n', end - p);
> 		if (!p)
> 			break;
> 	}

Ok, I now wrote

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

and you know what? Its logic seems wrong. The reason is that the p++ does not really have anything to do with the iteration, but rather steps past the '\n' from the memchr. So it's more like

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

So barring protests, that's what I'm going to use instead.

-- 
David Kastrup