threads / patch / 36911

patch, 2 partsblame: factor out get_next_line()

Subject: [PATCH 1/2] blame: factor out get_next_line()

## tl;dr

6 messages between Jun 13, 2014 and Jun 13, 2014. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

René Scharfe· Jun 13, 2014, 19:53 UTC · lore

Move the code for finding the start of the next line into a helper function in order to reduce duplication.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/blame.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

Show changes to builtin/blame.c +10 −18

diff --git a/builtin/blame.c b/builtin/blame.c
index a52a279..ad37edc 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2008,6 +2008,12 @@ static void output(struct scoreboard *sb, int option)
 	}
 }
 
+static const char *get_next_line(const char *start, const char *end)
+{
+	const char *nl = memchr(start, '\n', end - start);
+	return nl ? nl + 1 : NULL;
+}
+
 /*
  * To allow quick access to the contents of nth line in the
  * final image, prepare an index in the scoreboard.
@@ -2021,15 +2027,8 @@ static int prepare_lines(struct scoreboard *sb)
 	int *lineno;
 	int num = 0, incomplete = 0;
 
-	for (p = buf;;) {
-		p = memchr(p, '\n', end - p);
-		if (p) {
-			p++;
-			num++;
-			continue;
-		}
-		break;
-	}
+	for (p = get_next_line(buf, end); p; p = get_next_line(p, end))
+		num++;
 
 	if (len && end[-1] != '\n')
 		incomplete++; /* incomplete line at the end */
@@ -2038,15 +2037,8 @@ static int prepare_lines(struct scoreboard *sb)
 	lineno = sb->lineno;
 
 	*lineno++ = 0;
-	for (p = buf;;) {
-		p = memchr(p, '\n', end - p);
-		if (p) {
-			p++;
-			*lineno++ = p - buf;
-			continue;
-		}
-		break;
-	}
+	for (p = get_next_line(buf, end); p; p = get_next_line(p, end))
+		*lineno++ = p - buf;
 
 	if (incomplete)
 		*lineno++ = len;

-- 
2.0.0

René Scharfe· Jun 13, 2014, 19:54 UTC · re: René Scharfe · lore

[PATCH 2/2] blame: simplify prepare_lines()

Changing get_next_line() to return the end pointer instead of NULL in case no newline character is found treats allows us to treat complete and incomplete lines the same, simplifying the code. Switching to counting lines instead of EOLs allows us to start counting at the first character, instead of having to call get_next_line() first.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/blame.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

Show changes to builtin/blame.c +7 −13

diff --git a/builtin/blame.c b/builtin/blame.c
index ad37edc..662e3fe 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2011,7 +2011,7 @@ static void output(struct scoreboard *sb, int option)
 static const char *get_next_line(const char *start, const char *end)
 {
 	const char *nl = memchr(start, '\n', end - start);
-	return nl ? nl + 1 : NULL;
+	return nl ? nl + 1 : end;
 }
 
 /*
@@ -2025,25 +2025,19 @@ static int prepare_lines(struct scoreboard *sb)
 	const char *end = buf + len;
 	const char *p;
 	int *lineno;
-	int num = 0, incomplete = 0;
+	int num = 0;
 
-	for (p = get_next_line(buf, end); p; p = get_next_line(p, end))
+	for (p = buf; p < end; p = get_next_line(p, end))
 		num++;
 
-	if (len && end[-1] != '\n')
-		incomplete++; /* incomplete line at the end */
+	sb->lineno = lineno = xmalloc(sizeof(*sb->lineno) * (num + 1));
 
-	sb->lineno = xmalloc(sizeof(*sb->lineno) * (num + incomplete + 1));
-	lineno = sb->lineno;
-
-	*lineno++ = 0;
-	for (p = get_next_line(buf, end); p; p = get_next_line(p, end))
+	for (p = buf; p < end; p = get_next_line(p, end))
 		*lineno++ = p - buf;
 
-	if (incomplete)
-		*lineno++ = len;
+	*lineno = len;
 
-	sb->num_lines = num + incomplete;
+	sb->num_lines = num;
 	return sb->num_lines;
 }

-- 
2.0.0

Jonathan Nieder· Jun 13, 2014, 21:13 UTC · re: René Scharfe · lore

Re: [PATCH 2/2] blame: simplify prepare_lines()

René Scharfe wrote:

Show 6 quoted lines

> -	if (incomplete)
> -		*lineno++ = len;
> +	*lineno = len;
>  
> -	sb->num_lines = num + incomplete;
> +	sb->num_lines = num;

This will always treat whatever comes after the last newline as an incomplete line, even if it has zero length. Is that safe? (Not a rhetorical question --- I haven't looked carefully at the caller.)

Thanks, Jonathan

René Scharfe· Jun 13, 2014, 21:33 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/2] blame: simplify prepare_lines()

Am 13.06.2014 23:13, schrieb Jonathan Nieder:

Show 12 quoted lines

> René Scharfe wrote:
>
>> -	if (incomplete)
>> -		*lineno++ = len;
>> +	*lineno = len;
>>
>> -	sb->num_lines = num + incomplete;
>> +	sb->num_lines = num;
>
> This will always treat whatever comes after the last newline as an
> incomplete line, even if it has zero length.  Is that safe?  (Not a
> rhetorical question --- I haven't looked carefully at the caller.)

There is no need to look at the caller -- the contents of the lineno array is not (intended to be) changed by the patch.

The original code is:

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

Suppose there is no incomplete line. For the last EOL of a buffer, p points to buf[len - 1] after the memchr call. Then it is incremented. Then buf + len - buf is written into the last lineno member -- same as after the patch.

Makes sense?

René

Jonathan Nieder· Jun 13, 2014, 21:46 UTC · re: René Scharfe · lore

Re: [PATCH 2/2] blame: simplify prepare_lines()

Hi,

René Scharfe wrote:

> There is no need to look at the caller -- the contents of the lineno
> array is not (intended to be) changed by the patch.

Ah. If there is no incomplete line, the 'p < end' condition trips and it doesn't try to record the nonexistent incomplete line. Thanks for explaining and sorry for the confusion.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Jonathan Nieder· Jun 13, 2014, 21:05 UTC · re: René Scharfe · lore

Re: [PATCH 1/2] blame: factor out get_next_line()

René Scharfe wrote:

> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  builtin/blame.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[...]

> +	for (p = get_next_line(buf, end); p; p = get_next_line(p, end))
> +		num++;

This could say something like

	for (p = buf; p = get_next_line(buf, end); ) {
		num++;

or

	p = buf;
	while ((p = memchr(p, '\n', end - p))) {
		p++;
		num++;
	}

but what you have seems more readable.

Thanks, Jonathan