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

[Junio C Hamano <gitster@pobox.com>]

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;
	}

which I think is the prevalent style in our codebase.  The same for
the other loop we see in the new code below.

 - favor post-increment unless you use it as rvalue and need
   pre-increment;

 - SP around each binary ops e.g. 'end - p';

 - avoid assignments in conditionals when you do not have to.

> +	}
>  
> -	if (len && buf[len-1] != '\n')
> +	if (len && end[-1] != '\n')
>  		incomplete++; /* incomplete line at the end */

OK, so far we counted "num" complete lines and "incomplete" may be
one if there is an incomplete line after them.

> -	while (len--) {
> -		if (bol) {
> -			sb->lineno = xrealloc(sb->lineno,
> -					      sizeof(int *) * (num + 1));
> -			sb->lineno[num] = buf - sb->final_buf;
> -			bol = 0;
> -		}
> -		if (*buf++ == '\n') {
> -			num++;
> -			bol = 1;
> -		}
> +
> +	sb->lineno = lineno = xmalloc(sizeof(int) * (num + incomplete + 1));

OK, this function is called only once, so we know sb->lineno is NULL
originally and there is no reason to start from xrealloc().

> +	for (p = buf;;) {
> +		*lineno++ = p-buf;
> +		if ((p = memchr(p, '\n', end-p)) == NULL)
> +			break;
> +		++p;
>  	}
> -	sb->lineno = xrealloc(sb->lineno,
> -			      sizeof(int *) * (num + incomplete + 1));

These really *were* unnecessary reallocations.

Thanks for catching them, but this patch needs heavy style fixes.

> -	sb->lineno[num + incomplete] = buf - sb->final_buf;
> +
> +	if (incomplete)
> +		*lineno++ = len;
> +
>  	sb->num_lines = num + incomplete;
>  	return sb->num_lines;
>  }