blame: factor out get_next_line()

PATCH In review René Scharfe 2 participants 6 messages Jun 15, 2016

lore

Share & tools

lore

René Scharfe

author

2016-06-15 23:01 UTC #0 lore raw

PATCH 1/2 blame: factor out get_next_line()

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 <[email protected]>

---
 builtin/blame.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

builtin/blame.c +10 ~~-18~~

index a52a279..ad37edc 100644
@@ -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;

Signature · 1 line

2.0.0

Jonathan Nieder

Reviewed-by on v1 1/2

2016-06-15 23:01 UTC #1 lore raw

Quoted 4 lines · René Scharfe

Signed-off-by Rene Scharfe <[email protected]>
---
 builtin/blame.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

Reviewed-by Jonathan Nieder <[email protected]>

[...]

builtin/blame.c static int prepare_lines(struct scoreboard *sb) · line 2031

Quoted 2 lines

builtin/blame.c +2 -0

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

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

but what you have seems more readable.

Thanks,
Jonathan

René Scharfe

author

2016-06-15 23:01 UTC #2 lore raw

PATCH 2/2 blame: simplify prepare_lines()

In reply to René Scharfe

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 <[email protected]>

---
 builtin/blame.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

builtin/blame.c +7 ~~-13~~

index ad37edc..662e3fe 100644
@@ -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;
 }

Signature · 1 line

2.0.0

Jonathan Nieder

2016-06-15 23:01 UTC #3 lore raw

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

builtin/blame.c static int prepare_lines(struct scoreboard *sb) · line 2040

Quoted 6 lines · René Scharfe

builtin/blame.c +2 -3

-	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

author

2016-06-15 23:01 UTC #4 lore raw

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

Am 13.06.2014 23:13, schrieb Jonathan Nieder:

Quoted 6 lines

Quoted 4 lines · René Scharfe
builtin/blame.c +1 -2
-	if (incomplete)
-		*lineno++ = len;
+	*lineno = len;

Quoted 6 lines

builtin/blame.c static int prepare_lines(struct scoreboard *sb) · line 2040
Quoted 2 lines
builtin/blame.c +1 -1
-	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

Reviewed-by

2016-06-15 23:01 UTC #5 lore raw

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

Hi,

Quoted 2 lines · René Scharfe

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 <[email protected]>