David Kastrup <dak@xxxxxxx> 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@xxxxxxx>
> ---
> 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;
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html