can't find problem with memory

Discussion in 'C Programming' started by Steffen Loringer, Nov 16, 2005.

  1. Hi all,


    I'm using a linked list (double). The program is growing(windows xp task
    manager) if the showAllListNodes function is activated. But I can't
    figure out why. Any ideas???

    Thanks
    Steven

    my code is as follows:

    #include <stdio.h>
    #include <stdlib.h>

    typedef struct ListNode
    {
    int value;
    struct ListNode *previous;
    struct ListNode *next;
    };

    void newListNode(struct ListNode *temp_ls,int value);
    void showAllListNodes(void);
    void addListNode(int value);
    void removeLastListNode(void);

    struct ListNode *first_ls_ ;
    struct ListNode *last_ls_ ;

    int malloc_calls;
    int free_calls;

    void main(void)
    {
    int c = 1000;
    int i = 13;
    int x = 0;

    first_ls_= malloc(sizeof(struct ListNode));
    first_ls_->previous = NULL;
    first_ls_->value = 0;
    first_ls_->next = NULL;

    last_ls_ = first_ls_;
    first_ls_->next = last_ls_;

    for (x=0;x<99000;x++)
    {

    for (i=0;i<c;i++)
    {
    addListNode(i+1);
    }

    showAllListNodes();

    for (i=0;i<c;i++)
    {
    removeLastListNode();
    }

    }

    printf("\n Malloc Calls = %d \n",malloc_calls);
    printf("\n Free Calls = %d \n",free_calls);
    }

    void showAllListNodes()
    {
    struct ListNode *temp_ls = malloc(sizeof(struct ListNode));
    malloc_calls++;
    temp_ls = first_ls_;

    while(temp_ls != NULL)
    {
    //printf("\n %d =%d= %d
    Value=%d\n",temp_ls->previous,temp_ls,temp_ls->next,temp_ls->value);
    temp_ls = temp_ls->next;
    }

    free(temp_ls);
    free_calls++;

    }

    void addListNode(int value)
    {
    struct ListNode *temp_ls = malloc(sizeof(struct ListNode));
    malloc_calls++;
    temp_ls->previous = NULL;
    temp_ls->value = value;
    temp_ls->next = NULL;

    temp_ls->previous = last_ls_;
    last_ls_->next = temp_ls;
    last_ls_ = temp_ls;
    }

    void removeLastListNode(void)
    {
    last_ls_ = last_ls_->previous;
    free(last_ls_->next);
    free_calls++;
    last_ls_->next = NULL;

    }
    Steffen Loringer, Nov 16, 2005
    #1
    1. Advertising

  2. Steffen Loringer wrote:
    > Hi all,
    >
    >
    > I'm using a linked list (double). The program is growing(windows xp task
    > manager) if the showAllListNodes function is activated. But I can't
    > figure out why. Any ideas???
    >

    <snip>
    > void showAllListNodes()
    > {
    > struct ListNode *temp_ls = malloc(sizeof(struct ListNode));
    > malloc_calls++;
    > temp_ls = first_ls_;
    >


    temp_ls is allocated and leaked in those lines. After the assignment
    to temp_ls you no longer have a pointer to the allocated memory.

    -David
    David Resnick, Nov 16, 2005
    #2
    1. Advertising

  3. Thanks for respsonse, David,

    the problem occurs in that function at

    <snip>
    while(temp_ls != NULL)
    {
    //printf("\n %d
    =%d=Value=%d\n",temp_ls->previous,temp_ls,temp_ls->next,temp_ls->value);
    temp_ls = temp_ls->next;
    }


    If I deactviate the loop, the problem does'nt occur. If you only
    deactive printf expression the program is not growing.

    Thanks
    Steven
    Steffen Loringer, Nov 16, 2005
    #3
  4. Steffen Loringer wrote:
    > Thanks for respsonse, David,
    >
    > the problem occurs in that function at
    >
    > <snip>
    > while(temp_ls != NULL)
    > {
    > //printf("\n %d
    > =%d=Value=%d\n",temp_ls->previous,temp_ls,temp_ls->next,temp_ls->value);
    > temp_ls = temp_ls->next;
    > }
    >
    >
    > If I deactviate the loop, the problem does'nt occur. If you only
    > deactive printf expression the program is not growing.
    >
    > Thanks
    > Steven


    The place I showed you was a memory leak. You are also not freeing the
    thing that
    was allocated in that function. In abstract, you are doing this:

    char *foo = malloc(10);
    foo = bar;

    You no longer have a pointer to to the memory allocated, it can't ever
    be recovered.

    Your mistake is allocating anything at all for temp_ls. Why do you
    need
    a newly allocated node at all to iterate through your nodes?

    -David
    David Resnick, Nov 16, 2005
    #4
  5. Ok, found it...must read

    struct ListNode *temp_ls;

    instead of

    struct ListNode *temp_ls = malloc(sizeof(struct ListNode));

    Thanks a lot
    Steve
    Steffen Loringer, Nov 16, 2005
    #5

  6. > Your mistake is allocating anything at all for temp_ls. Why do you
    > need
    > a newly allocated node at all to iterate through your nodes?
    >
    > -David


    You are right, thanks. Any other recommendations for good coding practice?
    Steffen Loringer, Nov 16, 2005
    #6
  7. Steffen Loringer wrote:
    > Hi all,
    >


    Lower down the thread you asked for other comments
    on the code, here are some now that the leak is OK.

    >
    > I'm using a linked list (double). The program is growing(windows xp task
    > manager) if the showAllListNodes function is activated. But I can't
    > figure out why. Any ideas???
    >
    > Thanks
    > Steven
    >
    > my code is as follows:
    >
    > #include <stdio.h>
    > #include <stdlib.h>
    >
    > typedef struct ListNode
    > {
    > int value;
    > struct ListNode *previous;
    > struct ListNode *next;
    > };


    Um, what are you using typedef for here? Not needed.
    Unless you actually do what to typedef it, in which case
    you should provide a name for the new type!

    >
    > void newListNode(struct ListNode *temp_ls,int value);


    That isn't used, do you care?

    > void showAllListNodes(void);
    > void addListNode(int value);
    > void removeLastListNode(void);


    I'd make all these static unless they are going to be exposed in a
    header file.

    >
    > struct ListNode *first_ls_ ;
    > struct ListNode *last_ls_ ;


    Above should be static. I gather trailing _ is your way to indicate
    something
    is static? Many people use initial capital, but whatever.

    >
    > int malloc_calls;
    > int free_calls;


    Above should be static. And end with _ if that is your way to say they
    are global.

    >
    > void main(void)


    main should return int. That isn't a portable signature for main.

    > {
    > int c = 1000;
    > int i = 13;


    Strange magic numbers should be explained by a comment.
    And you set i to 0 anyway later.

    > int x = 0;
    >
    > first_ls_= malloc(sizeof(struct ListNode));


    malloc can fail, you should check
    Generally better malloc paradigm is
    first_ls_ = malloc(sizeof *first_ls_)
    Why? If you change the type of first_ls_ this still works...

    Same malloc comments elsewhere


    > first_ls_->previous = NULL;
    > first_ls_->value = 0;
    > first_ls_->next = NULL;
    >
    > last_ls_ = first_ls_;
    > first_ls_->next = last_ls_;


    I think you want = NULL here. If you iterate through the list at this
    point
    you'd loop.

    >
    > for (x=0;x<99000;x++)
    > {
    >
    > for (i=0;i<c;i++)
    > {
    > addListNode(i+1);
    > }
    >
    > showAllListNodes();
    >
    > for (i=0;i<c;i++)
    > {
    > removeLastListNode();
    > }
    >
    > }
    >
    > printf("\n Malloc Calls = %d \n",malloc_calls);
    > printf("\n Free Calls = %d \n",free_calls);
    > }
    >
    > void showAllListNodes()
    > {
    > struct ListNode *temp_ls = malloc(sizeof(struct ListNode));


    As seen elsewhere, you don't want to malloc this

    > malloc_calls++;
    > temp_ls = first_ls_;
    >
    > while(temp_ls != NULL)
    > {
    > //printf("\n %d =%d= %d
    > Value=%d\n",temp_ls->previous,temp_ls,temp_ls->next,temp_ls->value);


    Should print pointers to %p, and cast to (void*).

    > temp_ls = temp_ls->next;
    > }
    >
    > free(temp_ls);


    Don't want to free this either

    > free_calls++;
    >
    > }
    >
    > void addListNode(int value)
    > {
    > struct ListNode *temp_ls = malloc(sizeof(struct ListNode));
    > malloc_calls++;
    > temp_ls->previous = NULL;
    > temp_ls->value = value;
    > temp_ls->next = NULL;
    >
    > temp_ls->previous = last_ls_;
    > last_ls_->next = temp_ls;
    > last_ls_ = temp_ls;
    > }
    >
    > void removeLastListNode(void)
    > {
    > last_ls_ = last_ls_->previous;
    > free(last_ls_->next);
    > free_calls++;
    > last_ls_->next = NULL;


    If you are removing the first list node, seems like last_ls_ will be
    NULL
    here and you have troubles dereferencing it.

    >
    > }
    David Resnick, Nov 16, 2005
    #7
  8. Steffen Loringer

    Randy Howard Guest

    Steffen Loringer wrote
    (in article <>):

    >
    >> Your mistake is allocating anything at all for temp_ls. Why do you
    >> need
    >> a newly allocated node at all to iterate through your nodes?
    >>
    >> -David

    >
    > You are right, thanks. Any other recommendations for good coding practice?


    Yes, stop using 'void main(void)' unless you are working on an
    embedded project (unlikely, since you are using malloc() and
    friends).

    It doesn't matter what your C book or your instructor say, 'void
    main...' is not legit in standard C apart from some embedded
    scenarios.

    If you do not intend to use command line arguments at all, then
    int main(void)
    is the proper form.

    --
    Randy Howard (2reply remove FOOBAR)
    "The power of accurate observation is called cynicism by those
    who have not got it." - George Bernard Shaw
    Randy Howard, Nov 19, 2005
    #8
    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. ad
    Replies:
    2
    Views:
    849
  2. DR
    Replies:
    0
    Views:
    647
  3. Wybo Dekker
    Replies:
    1
    Views:
    341
    Yukihiro Matsumoto
    Nov 15, 2005
  4. vdvorkin
    Replies:
    0
    Views:
    390
    vdvorkin
    Feb 10, 2011
  5. vdvorkin
    Replies:
    3
    Views:
    800
    vdvorkin
    Feb 14, 2011
Loading...

Share This Page