can't find problem with memory

[Steffen Loringer]

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;

}

 

[David Resnick]

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

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

 

[Steffen Loringer]

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

 

[David Resnick]

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

 

[Steffen Loringer]

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]

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?

 

[David Resnick]

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.

 

[Randy Howard]

Steffen Loringer wrote

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.