Hi Martijn,
this is a nice simplification, OK schwarze@.
A few nits:
* The MAXIMUM() macro is now unused, so i prefer that you delete
the definition.
* The second getline(3) argument should be size_t, not u_long,
so change that in the struct declaration (it's not used anywhere
else). Not sure whether your type mismatch might cause trouble
on any of our platforms, or if the two types are identical
everywhere. While there, make the comment more intelligible.
* From one comment, delete "and copy line" which is no longer true.
I'm appending the version of your patch that i tested.
Todd C. Miller wrote on Wed, Jul 22, 2020 at 01:19:47PM -0600:
> On Wed, 22 Jul 2020 20:29:06 +0200, Martijn van Duren wrote:
>> /* Remove trailing newline, if it exists, and copy line. */
>> - if (line[len - 1] == '\n')
>> + if (lp->line[len - 1] == '\n')
>> len--;
>> lp->line[len] = '\0';
> You could replace "len--" with "lp->line[len - 1] = '\0'" here (or
> "lp->line[--len] = '\0'" if you prefer but len is otherwise unused).
> Then there would be no need to NUL terminate here since getline(3)
> always NUL terminates.
Indeed. I think this form is clearest, shortest, and safest:
/* Remove the trailing newline, if any. */
if (lp->line[len - 1] == '\n')
p->line[--len] = '\0';
Yours,
Ingo
Index: join.c
===================================================================
RCS file: /cvs/src/usr.bin/join/join.c,v
retrieving revision 1.32
diff -u -p -r1.32 join.c
--- join.c 14 Nov 2018 15:16:09 -0000 1.32
+++ join.c 23 Jul 2020 14:47:06 -0000
@@ -43,8 +43,6 @@
#include <unistd.h>
#include <wchar.h>
-#define MAXIMUM(a, b) (((a) > (b)) ? (a) : (b))
-
/*
* There's a structure per input file which encapsulates the state of the
* file. We repeatedly read lines from each file until we've read in all
@@ -53,7 +51,7 @@
*/
typedef struct {
char *line; /* line */
- u_long linealloc; /* line allocated count */
+ size_t linealloc; /* bytes allocated for line */
char **fields; /* line field(s) */
u_long fieldcnt; /* line field(s) count */
u_long fieldalloc; /* line field(s) allocated count */
@@ -271,9 +269,8 @@ slurp(INPUT *F)
{
LINE *lp, *lastlp, tmp;
ssize_t len;
- size_t linesize;
u_long cnt;
- char *bp, *fieldp, *line;
+ char *bp, *fieldp;
/*
* Read all of the lines from an input file that have the same
@@ -281,8 +278,6 @@ slurp(INPUT *F)
*/
F->setcnt = 0;
- line = NULL;
- linesize = 0;
for (lastlp = NULL; ; ++F->setcnt) {
/*
* If we're out of space to hold line structures, allocate
@@ -320,23 +315,12 @@ slurp(INPUT *F)
F->pushbool = 0;
continue;
}
- if ((len = getline(&line, &linesize, F->fp)) == -1)
+ if ((len = getline(&(lp->line), &(lp->linealloc), F->fp)) == -1)
break;
- /* Remove trailing newline, if it exists, and copy line. */
- if (line[len - 1] == '\n')
- len--;
- if (lp->linealloc <= len + 1) {
- char *p;
- u_long newsize = lp->linealloc +
- MAXIMUM(100, len + 1 - lp->linealloc);
- if ((p = realloc(lp->line, newsize)) == NULL)
- err(1, NULL);
- lp->line = p;
- lp->linealloc = newsize;
- }
- memcpy(lp->line, line, len);
- lp->line[len] = '\0';
+ /* Remove the trailing newline, if any. */
+ if (lp->line[len - 1] == '\n')
+ lp->line[--len] = '\0';
/* Split the line into fields, allocate space as necessary. */
lp->fieldcnt = 0;
@@ -363,7 +347,6 @@ slurp(INPUT *F)
break;
}
}
- free(line);
}
char *
