Re: malloc()/realloc() - have I got this right?

Discussion in 'C Programming' started by Jens Thoms Toerring, May 28, 2008.

  1. Dave <> wrote:
    > I'm teaching myself C by working my way through Steve Summit's tutorial
    > (http://www.eskimo.com/~scs/cclass/cclass.html). In one of the questions
    > (assignment 6, exercise 7), you have to write a function to read lines of
    > arbitrary length from the command line, using malloc() and realloc() to
    > allocate the necessary memory to hold the lines. I came up with this:


    It would be better if you also would show which header files
    you're including, it's not uncommon that a few essential ones
    get forgotten...

    > char *getline(char *line)


    Why do you pass 'line' to the function if you don't use its
    value anywhere?

    > {
    > int line_len = 0;
    > int max_len = 32; /* initial guess */
    > char *start; /* pointer to start of string is the return value */
    > char *count;


    I would recommend to rename that to something more fitting
    like 'current_pointer' or 'next' or maybe 'end' something
    like that. It helps in the long run to have variable names
    that quickly tell you what they are used for. And 'count'
    sounds a lot like a counter variable, not a pointer.

    > int c;


    > line = malloc(max_len * sizeof(char));


    You could drop the 'sizeof(char)' since it's guaranteed to
    be 1. Or replace it by 'sizeof *line'.

    > if (line == NULL)
    > {
    > printf("Out of memory.\n");
    > exit(1);
    > }
    > start = line;
    > count = line;


    > while ((c = getchar()) != EOF)
    > {
    > if (c == '\n')
    > break;
    > if (line_len == max_len - 1)
    > {
    > max_len *= 2;
    > line = realloc(line, max_len * sizeof(char));
    > if (line == NULL)
    > {
    > printf("Out of memory.\n");
    > exit(1);
    > }


    What you're not taking into account here is that realloc()
    could set 'line' to a new value. If that happens you also
    have to readjust 'count' by I guess

    count = line + line_len;

    > }


    > *count++ = c;
    > line_len++;
    > }
    > *count = '\0';
    > return start;


    And here is another potential problem lurking. If 'line' got
    changed to a new value during one of the realloc() calls you
    will return a pointer to some memory that you don't own any-
    more. You can drop the 'start' variable completely and in-
    stead simply return 'line'.

    Another thing you could do before returning (if you don't want
    to waste any memory) is to realloc() 'line' to the exact length
    you needed for the string.

    > }


    > And I'm using this to test it:


    > int main()
    > {
    > char *line;
    > int i;
    > for (i = 0; i < 5; i++)
    > {
    > line = getline(line);
    > printf("%s\n%d\n", line, strlen(line));


    If I switch to very picky mode I would point out that strlen()
    returns a size_t value, so using "%d" to print it is not
    strictly correct. I would probably use

    printf("%s\n%lu\n", line, (unsigned long) strlen(line));

    (unless you have a C999 compiler where you have a format
    specifier for size_t).

    > }
    > return 0;
    > }


    > On Linux it seems to work with lines of any length. On Windows (using the
    > MinGW compiler) it fails if the line is longer than 1024 characters (just
    > prints out a few random characters and reports the length as 0). I'm
    > hoping someone can tell me if I've got this function basically right -
    > have I introduced some bug which is causing the crash, or is it a Windows
    > thing that I don't need to concern myself with?


    I can only guess but it might be related to the problem that you
    don't consider that realloc() might return a different pointer
    for the memory.

    > Also, is it better in principle to make more calls to realloc() to ensure
    > that you get exactly the memory you need and no more, or to make fewer
    > calls and just malloc() more than you think you'll need at the beginning?


    You can always reduce the memory by using realloc() in the end.
    malloc() and realloc() can be rather expensive functions, so if
    you really have to worry about execution speed you may get some
    speed-up if you try to reduce the number of calls of these func-
    tions. But that's something you should measure, not just assume.

    Regards, Jens
    --
    \ Jens Thoms Toerring ___
    \__________________________ http://toerring.de
    Jens Thoms Toerring, May 28, 2008
    #1
    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. Bren
    Replies:
    8
    Views:
    2,011
    Stephen Howe
    Sep 4, 2003
  2. DrBob
    Replies:
    2
    Views:
    576
    Unforgiven
    Nov 26, 2003
  3. Jun Woong

    Re: Override malloc,calloc,realloc and free?

    Jun Woong, Jun 26, 2003, in forum: C Programming
    Replies:
    0
    Views:
    1,069
    Jun Woong
    Jun 26, 2003
  4. Keith Thompson

    Re: malloc()/realloc() - have I got this right?

    Keith Thompson, May 30, 2008, in forum: C Programming
    Replies:
    17
    Views:
    496
    Keith Thompson
    May 31, 2008
  5. Flash Gordon

    Re: malloc()/realloc() - have I got this right?

    Flash Gordon, May 31, 2008, in forum: C Programming
    Replies:
    4
    Views:
    262
    Joachim Schmitz
    Jun 1, 2008
Loading...

Share This Page