Some suggestion required regarding get line functionality.

Discussion in 'C Programming' started by somenath, Nov 5, 2007.

  1. somenath

    somenath Guest

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

    }
     
    somenath, Nov 5, 2007
    #1
    1. Advertising

  2. somenath

    jacob navia Guest

    somenath wrote:
    > 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.


    > }
    >



    --
    jacob navia
    jacob at jacob point remcomp point fr
    logiciels/informatique
    http://www.cs.virginia.edu/~lcc-win32
     
    jacob navia, Nov 5, 2007
    #2
    1. Advertising

  3. somenath

    pete Guest

    somenath wrote:
    >
    > 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.

    --
    pete
     
    pete, Nov 5, 2007
    #3
  4. somenath

    Flash Gordon Guest

    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.
    --
    Flash Gordon
     
    Flash Gordon, Nov 5, 2007
    #4
  5. somenath

    pete Guest

    jacob navia wrote:
    >
    > somenath wrote:
    > > 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;


    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.

    >
    > > 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.


    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.

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


    --
    pete
     
    pete, Nov 5, 2007
    #5
  6. somenath

    somenath Guest

    On Nov 5, 5:53 pm, jacob navia <> wrote:
    > somenath wrote:
    > > 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.
     
    somenath, Nov 5, 2007
    #6
  7. somenath

    Eric Sosman Guest

    somenath wrote:
    > 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.

    > }
    >



    --
    Eric Sosman
    lid
     
    Eric Sosman, Nov 5, 2007
    #7
  8. somenath

    somenath Guest

    On Nov 5, 6:34 pm, pete <> wrote:
    > jacob navia wrote:
    >
    > > somenath wrote:
    > > > 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;

    >
    > 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.
    >
    >
    >
    > > > 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.

    >
    > 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.
    >
    > > That is not a BIG SIN. You could just return
    > > what you have read so far.

    >
     
    somenath, Nov 5, 2007
    #8
  9. somenath

    Mark Bluemel Guest

    somenath wrote:
    > On Nov 5, 6:34 pm, pete <> wrote:
    >> jacob navia wrote:
    >>
    >>> somenath wrote:
    >>>> 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;

    >> 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 .


    What is the prototype for malloc()?
     
    Mark Bluemel, Nov 5, 2007
    #9
  10. somenath

    somenath Guest

    On Nov 5, 6:56 pm, Mark Bluemel <> wrote:
    > somenath wrote:
    > > On Nov 5, 6:34 pm, pete <> wrote:
    > >> jacob navia wrote:

    >
    > >>> somenath wrote:
    > >>>> 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;
    > >> 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 .

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

    Many thanks . I got the answer.
     
    somenath, Nov 5, 2007
    #10
  11. somenath

    CBFalconer Guest

    somenath wrote:
    >
    > 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/>

    --
    Chuck F (cbfalconer at maineline dot net)
    <http://cbfalconer.home.att.net>
    Try the download section.



    --
    Posted via a free Usenet account from http://www.teranews.com
     
    CBFalconer, Nov 6, 2007
    #11
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Trevor Benedict R
    Replies:
    1
    Views:
    400
    MasterGaurav
    Oct 23, 2005
  2. Piers Chivers
    Replies:
    2
    Views:
    395
    Piers Chivers
    Mar 2, 2004
  3. Sandeep Arya

    Threads: Issue and suggestion required

    Sandeep Arya, Jul 19, 2005, in forum: Python
    Replies:
    1
    Views:
    289
    Peter Hansen
    Jul 19, 2005
  4. kaushikshome
    Replies:
    4
    Views:
    773
    kaushikshome
    Sep 10, 2006
  5. Srijayanth Sridhar

    Naming suggestion required

    Srijayanth Sridhar, Jun 12, 2009, in forum: Ruby
    Replies:
    3
    Views:
    107
    Srijayanth Sridhar
    Jun 12, 2009
Loading...

Share This Page