patterncMinor
Readline function
Viewed 0 times
functionreadlinestackoverflow
Problem
I have developed a
readline function in C and I want to know your opinion on it:// read a the whole line, stop if encouter eof
static char *readline(FILE *in, sid_error *error)
{
#define MAX_SIZE 64
size_t size = MAX_SIZE;
char *str;
if((str = malloc(sizeof *str * size)) == NULL) {
*error = ERROR_MALLOC;
return NULL;
}
char *pos;
char read = 0;
while(fgets(str + size - MAX_SIZE, MAX_SIZE, in)) {
read = 1;
if(pos = strchr(str, '\n')) {
*pos = '\0';
break;
}
size = size + MAX_SIZE - 1;
str = realloc(str, sizeof *str * size);
}
if(!read) {
free(str);
return NULL;
}
return str;
}Solution
-
Inconsistent memory allocation. Code checks for a
-
Reliance on no embedded
-
Minor performance issue. In searching for the final
-
Reallocation increments the buffer size in a linear fashion. Increasing exponentially (example: 2x) has advantages with unusually long lines. Yet for typically applications without insanely long lines, either approach is OK.
-
Nice correct use of
[Edit]
Inconsistent memory allocation. Code checks for a
NULL return on malloc(), yet no checks on realloc(). Recommend adding a NULL check on realloc().-
Reliance on no embedded
'\0'. The following code searches up to '\0' for a '\n' to determine if reading the line is done. In unusual situations, a '\0' will occur before the final '\n' + '\0' pair. So if MAX_SIZE was a larger value, the same file input (with an embedded '\0') could read more of the line than this code. This is a weakness of fgets() and this readline() inherits it. To get around this, some function other that fgets() needs to be used for primary input.if(pos = strchr(str, '\n'))-
Minor performance issue. In searching for the final
'\n', trimming fgets() is usually faster with strlen() than strchr(). But more importantly, this code re-searches the same buffer unnecessarily. Consider instead the following.// if(pos = strchr(str, '\n')) {
if(pos = strchr(str + size - MAX_SIZE, '\n')) {-
Reallocation increments the buffer size in a linear fashion. Increasing exponentially (example: 2x) has advantages with unusually long lines. Yet for typically applications without insanely long lines, either approach is OK.
-
Nice correct use of
size_t for buffer size.[Edit]
- If there is the unusual embedded
'\0'in input such as"abc\0xyx\n\0", then the while loop will unfortunately read the rest of the file. The while loop will not exit withif(pos = strchr(str, '\n'))and will continue reading untilfgets()returnsNULL. As in point #3, subsequent searches for\nshould begin with the new data read and not at&str[0]. Further, looping should only occur if'\n'was not found and buffer is full.
Code Snippets
if(pos = strchr(str, '\n'))// if(pos = strchr(str, '\n')) {
if(pos = strchr(str + size - MAX_SIZE, '\n')) {Context
StackExchange Code Review Q#78571, answer score: 6
Revisions (0)
No revisions yet.