By using this site, you agree to our updated Privacy Policy and our Terms of Use. Manage your Cookies Settings.
454,504 Members | 1,724 Online
Bytes IT Community
+ Ask a Question
Need help? Post your question and get tips & solutions from a community of 454,504 IT Pros & Developers. It's quick & easy.

can't find problem with memory

P: n/a
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;

}

Nov 16 '05 #1
Share this Question
Share on Google+
7 Replies


P: n/a

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

Nov 16 '05 #2

P: n/a
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

Nov 16 '05 #3

P: n/a
Ok, found it...must read

struct ListNode *temp_ls;

instead of

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

Thanks a lot
Steve

Nov 16 '05 #4

P: n/a

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

Nov 16 '05 #5

P: n/a
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?

Nov 16 '05 #6

P: n/a

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.

}


Nov 16 '05 #7

P: n/a
Steffen Loringer wrote
(in article <3u************@news.dfncis.de>):
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

Nov 19 '05 #8

This discussion thread is closed

Replies have been disabled for this discussion.