Some suggestion required regarding get line functionality.

[somenath]

Hi All,

I was trying to write a function which will read one line from a
specified file and return the line. It is currently working fine.
But it would be very much helpful for me if some one point out some
improvement points or some error in the code.

Regards,
Somenath

char* get_line(FILE *fp)
{
int len = 2; /*First memory is allocated for 2 bytes 1 for one
character another for '\0'*/
int pos = 0;
char *buff;
char *temp_buff;
char c;
buff = malloc(len);
if (fp == NULL || buff == NULL)
{
return (NULL);
}
while ( (c = fgetc(fp)) != EOF)
{
buff[pos++] = c;
if (c == '\n')
{
/*pos is decremented by one because, currently buff[pos]
contains newline at the
position of pos
but we need to put newline at the end od buf[pos] ;So
pos is decremented by one and
at the end before
return we are assignning '\0' at the place of newline i.e
'\n' is replcaed by '\0'.*/
pos = pos-1;
/*We reached the end of the line*/

break;
}
if (pos == len)
{

len += 2;
temp_buff = realloc(buff,len);
if (temp_buff == NULL)
{
/*Reallocation of memory fails */
return NULL;
}
else
{

buff = temp_buff;
}
}
}
buff[pos] = '\0';
return buff = (c==EOF?NULL:buff);

}

 

[jacob navia]

Hi All,

I was trying to write a function which will read one line from a
specified file and return the line. It is currently working fine.
But it would be very much helpful for me if some one point out some
improvement points or some error in the code.

Regards,
Somenath

char* get_line(FILE *fp)
{
int len = 2; /*First memory is allocated for 2 bytes 1 for one
character another for '\0'*/
int pos = 0;
char *buff;
char *temp_buff;
char c;
buff = malloc(len);
if (fp == NULL || buff == NULL)
{
return (NULL);
}
while ( (c = fgetc(fp)) != EOF)
{
buff[pos++] = c;
if (c == '\n')
{
/*pos is decremented by one because, currently buff[pos]
contains newline at the
position of pos
but we need to put newline at the end od buf[pos] ;So
pos is decremented by one and
at the end before
return we are assignning '\0' at the place of newline i.e
'\n' is replcaed by '\0'.*/
pos = pos-1;
/*We reached the end of the line*/

break;
}

1)
I think itr would be easier to read to test FIRST for
'\n', THEN add to the buffer if that is the case

if (c == '\n')
break;
buf[pos++] = c;

2)
Under windows, you have new lines with \r \n. You should
test for \r if you want to run under windows and the file
you are reading is opened in binary mode.

if (pos == len)
{

len += 2;

It would be more efficient to reallocate much more than
just 2 characters, to avoid too many calls to
realloc.

temp_buff = realloc(buff,len);
if (temp_buff == NULL)
{
/*Reallocation of memory fails */

You have a memory leak here. You return without freeing
the old buffer...

return NULL;
}
else
{

buff = temp_buff;
}
}
}
buff[pos] = '\0';
return buff = (c==EOF?NULL:buff);

Why return NULL if you hit EOF?

That means that you got a file that doesn't end in a
newline. That is not a BIG SIN. You could just return
what you have read so far.

 

[pete]

Hi All,

I was trying to write a function which will read one line from a
specified file and return the line. It is currently working fine.
But it would be very much helpful for me if some one point out some
improvement points or some error in the code.

Regards,

char c;

while ( (c = fgetc(fp)) != EOF)

Right off the bat, (c) should be type int.

 

[Flash Gordon]

somenath wrote, On 05/11/07 12:43:

Hi All,

I was trying to write a function which will read one line from a
specified file and return the line. It is currently working fine.
But it would be very much helpful for me if some one point out some
improvement points or some error in the code.

Regards,
Somenath

char* get_line(FILE *fp)
{
int len = 2; /*First memory is allocated for 2 bytes 1 for one
character another for '\0'*/
int pos = 0;
char *buff;
char *temp_buff;
char c;
buff = malloc(len);
if (fp == NULL || buff == NULL)
{
return (NULL);
}

You have a memory leak if fp was NULL and malloc succeeded. Test fp
before doing the malloc.

while ( (c = fgetc(fp)) != EOF)

c should have been declared as an int rather than a char. If char is
unsigned this test will not detect EOF and if char is signed at least
one potentially valid character could be detected as EOF.

{
buff[pos++] = c;
if (c == '\n')
{
/*pos is decremented by one because, currently buff[pos]
contains newline at the
position of pos
but we need to put newline at the end od buf[pos] ;So
pos is decremented by one and
at the end before
return we are assignning '\0' at the place of newline i.e
'\n' is replcaed by '\0'.*/
pos = pos-1;

One of "pos -= 1", "pos--" or "--pos" would be clearer in my opinion.

/*We reached the end of the line*/

break;
}
if (pos == len)
{

len += 2;
temp_buff = realloc(buff,len);
if (temp_buff == NULL)
{
/*Reallocation of memory fails */

This is a memory leak since buf still points to allocated memory.

return NULL;
}
else
{

buff = temp_buff;
}
}
}
buff[pos] = '\0';
return buff = (c==EOF?NULL:buff);

This is another memory leak when the end of file is reached or a an
error occurs.

}

In general you should not throw away the successfully read data if an
error occurs. You should return what you have and also indicate that a
failure has occurred. It is also subject to a DoS (Denial of Service)
attack by feeding it a continuous stream or data that does not include a
new line.

 

[pete]

Those int should be size_t.

char *buff;
char *temp_buff;
char c;
buff = malloc(len);
if (fp == NULL || buff == NULL)
{
return (NULL);
}
while ( (c = fgetc(fp)) != EOF)
{
buff[pos++] = c;
if (c == '\n')
{
/*pos is decremented by one because, currently buff[pos]
contains newline at the
position of pos
but we need to put newline at the end od buf[pos] ;So
pos is decremented by one and
at the end before
return we are assignning '\0' at the place of newline i.e
'\n' is replcaed by '\0'.*/
pos = pos-1;
/*We reached the end of the line*/

break;
}

1)
I think itr would be easier to read to test FIRST for
'\n', THEN add to the buffer if that is the case

if (c == '\n')
break;
buf[pos++] = c;

I think so too.

2)
Under windows, you have new lines with \r \n. You should
test for \r if you want to run under windows and the file
you are reading is opened in binary mode.

The whole concept of "line" as in "get_line"
applies to streams opened in text mode.

It would be more efficient to reallocate much more than
just 2 characters, to avoid too many calls to
realloc.

How much more?
How do you prevent eventual overflow in (len)?

temp_buff = realloc(buff,len);
if (temp_buff == NULL)
{
/*Reallocation of memory fails */

You have a memory leak here. You return without freeing
the old buffer...

return NULL;
}
else
{

buff = temp_buff;
}
}
}
buff[pos] = '\0';
return buff = (c==EOF?NULL:buff);

There's no point in assigning a value to (buff) either,
in the return statement.

Why return NULL if you hit EOF?

That means that you got a file that doesn't end in a
newline.

No.
It could also mean that the get_line function
had read the last character of the file,
on the previous invocation of get_line.

get_line should have some way to indicate
that the end of file has been reached.
Returning NULL is OK.

 

[somenath]

Hi All,

I was trying to write a function which will read one line from a
specified file and return the line. It is currently working fine.
But it would be very much helpful for me if some one point out some
improvement points or some error in the code.

Regards,
Somenath

char* get_line(FILE *fp)
{
int len = 2; /*First memory is allocated for 2 bytes 1 for one
character another for '\0'*/
int pos = 0;
char *buff;
char *temp_buff;
char c;
buff = malloc(len);
if (fp == NULL || buff == NULL)
{
return (NULL);
}
while ( (c = fgetc(fp)) != EOF)
{
buff[pos++] = c;
if (c == '\n')
{
/*pos is decremented by one because, currently buff[pos]
contains newline at the
position of pos
but we need to put newline at the end od buf[pos] ;So
pos is decremented by one and
at the end before
return we are assignning '\0' at the place of newline i.e
'\n' is replcaed by '\0'.*/
pos = pos-1;
/*We reached the end of the line*/

break;
}

1)
I think itr would be easier to read to test FIRST for
'\n', THEN add to the buffer if that is the case

if (c == '\n')
break;
buf[pos++] = c;

2)
Under windows, you have new lines with \r \n. You should
test for \r if you want to run under windows and the file
you are reading is opened in binary mode.

if (pos == len)
{

len += 2;

It would be more efficient to reallocate much more than
just 2 characters, to avoid too many calls to
realloc.

temp_buff = realloc(buff,len);
if (temp_buff == NULL)
{
/*Reallocation of memory fails */

You have a memory leak here. You return without freeing
the old buffer...

return NULL;
}
else
{

buff = temp_buff;
}
}
}
buff[pos] = '\0';
return buff = (c==EOF?NULL:buff);

Why return NULL if you hit EOF?

That means that you got a file that doesn't end in a
newline. That is not a BIG SIN. You could just return
what you have read so far.

}

Many thanks for the information. Actually I wanted same return value
for failure condition i.e NULL .And I wanted to use the this get_line
function as
while((buff= get_line(fp))!=NULL) .If I returned EOF then I did not
find any way to use the get_line function as I wanted.
I would be great help for me if you throw some light so that inspite
of returning EOF still I could use it as I mentioned earlier.

 

[Eric Sosman]

Hi All,

I was trying to write a function which will read one line from a
specified file and return the line. It is currently working fine.
But it would be very much helpful for me if some one point out some
improvement points or some error in the code.

Regards,
Somenath

char* get_line(FILE *fp)
{
int len = 2; /*First memory is allocated for 2 bytes 1 for one
character another for '\0'*/
int pos = 0;
char *buff;
char *temp_buff;
char c;
buff = malloc(len);
if (fp == NULL || buff == NULL)
{
return (NULL);

If fp is NULL and the malloc() succeeded, this leaks
the allocated memory.

}
while ( (c = fgetc(fp)) != EOF)

Unreliable test; see the Question 12.1 in the FAQ.

{
buff[pos++] = c;
if (c == '\n')
{
/*pos is decremented by one because, currently buff[pos]
contains newline at the
position of pos

Comment incorrect: buff[pos] contains indeterminate garbage.
buff[pos-1] holds a newline.

but we need to put newline at the end od buf[pos] ;So
pos is decremented by one and
at the end before
return we are assignning '\0' at the place of newline i.e
'\n' is replcaed by '\0'.*/
pos = pos-1;
/*We reached the end of the line*/

break;
}
if (pos == len)
{

len += 2;
temp_buff = realloc(buff,len);

The parsimonious growth strategy is likely to use a good
deal more compute power than a super-linear strategy would.

if (temp_buff == NULL)
{
/*Reallocation of memory fails */
return NULL;

This leaks the previous buffer.

}
else
{

buff = temp_buff;
}
}
}
buff[pos] = '\0';
return buff = (c==EOF?NULL:buff);

This leaks the buffer if EOF was detected. Also, the
assignment is useless.

 

[somenath]

Those int should be size_t.

Thanks for the information . But I would request you to explain why
those two variables should be size_t ? According to my knowledge
size_t is type for objects declared to store result of sizeof
operator. Please explain .

char *buff;
char *temp_buff;
char c;
buff = malloc(len);
if (fp == NULL || buff == NULL)
{
return (NULL);
}
while ( (c = fgetc(fp)) != EOF)
{
buff[pos++] = c;
if (c == '\n')
{
/*pos is decremented by one because, currently buff[pos]
contains newline at the
position of pos
but we need to put newline at the end od buf[pos] ;So
pos is decremented by one and
at the end before
return we are assignning '\0' at the place of newline i.e
'\n' is replcaed by '\0'.*/
pos = pos-1;
/*We reached the end of the line*/
break;
}

1)
I think itr would be easier to read to test FIRST for
'\n', THEN add to the buffer if that is the case

if (c == '\n')
break;
buf[pos++] = c;

I think so too.

2)
Under windows, you have new lines with \r \n. You should
test for \r if you want to run under windows and the file
you are reading is opened in binary mode.

The whole concept of "line" as in "get_line"
applies to streams opened in text mode.

It would be more efficient to reallocate much more than
just 2 characters, to avoid too many calls to
realloc.

How much more?
How do you prevent eventual overflow in (len)?

You have a memory leak here. You return without freeing
the old buffer...

return NULL;
}
else
{
buff = temp_buff;
}
}
}
buff[pos] = '\0';
return buff = (c==EOF?NULL:buff);

There's no point in assigning a value to (buff) either,
in the return statement.

Why return NULL if you hit EOF?

That means that you got a file that doesn't end in a
newline.

No.
It could also mean that the get_line function
had read the last character of the file,
on the previous invocation of get_line.

get_line should have some way to indicate
that the end of file has been reached.
Returning NULL is OK.

That is not a BIG SIN. You could just return
what you have read so far.

 

[Mark Bluemel]

Thanks for the information . But I would request you to explain why
those two variables should be size_t ? According to my knowledge
size_t is type for objects declared to store result of sizeof
operator. Please explain .

What is the prototype for malloc()?

 

[somenath]

What is the prototype for malloc()?- Hide quoted text -

Many thanks . I got the answer.

 

[CBFalconer]

I was trying to write a function which will read one line from a
specified file and return the line. It is currently working fine.
But it would be very much helpful for me if some one point out
some improvement points or some error in the code.

Try ggets (and fggets) available in ggets.zip (standard C) at:

<http://cbfalconer.home.att.net/download/>