Printable View
Shouldn't this become NULL and exit the loop? It certainly doesn't...
Code:void DestroySymTab(struct SymTab *ATable) {
int i;
struct SymEntry *p;
for (i = 0; i < ATable->Size; i++) {
while(ATable->Contents[i] != NULL) {
p = (struct SymEntry*)ATable->Contents[i];
while(p->Next != NULL){
p = p->Next;
}
free(p);
p = NULL;
}
}
free(ATable);
};
Are you sure that your linked list is actually NULL-terminated, and doesn't have a loop somewhere?
--
Mats
Only if one day you have a Next that is NULL already.Quote:
Originally Posted by P3nGu1N
Well, right now for testing purposes I haven't implemented adding anything past one name, and SymEntry->Next should be NULL terminated for that one.
I think it shouldn't even touch that second while loop?... maybe I should comment it out and see if it makes a difference.Code:bool EnterName(struct SymTab *ATable, const char *Name,
struct SymEntry * *AnEntry) {
int hashValue = hash(Name,ATable);
char n[200];
int nameLength;
struct SymEntry *currentEntry;
strcpy(n, Name);
nameLength = strlen(n) + 1;
if(ATable->Contents[hashValue] == NULL) {
currentEntry = (struct SymEntry*)malloc(sizeof(struct SymEntry*) + nameLength);
currentEntry->Name = n;
currentEntry->Next = NULL;
ATable->Contents[hashValue] = currentEntry;
AnEntry = ¤tEntry;
return false;
}
return true;
};
Have you tried one of these inside that while loop:
Code:printf("address: %p\n",p->Next); fflush(stdout);
No, there's a different problem:
If the red bit is true the first iteration, what changes it later?Code:while(ATable->Contents[i] != NULL) {
p = (struct SymEntry*)ATable->Contents[i];
while(p->Next != NULL){
p = p->Next;
}
free(p);
p = NULL;
}
--
Mats
Tried it, and nope the infinite loop is in the outer while statement.
Code:void DestroySymTab(struct SymTab *ATable) {
int i;
struct SymEntry *p;
for (i = 0; i < ATable->Size; i++) {
while(ATable->Contents[i] != NULL) {
p = (struct SymEntry*)ATable->Contents[i];
/*while(p->Next != NULL){
p = p->Next;
}*/
free(p);
p = NULL;
}
}
free(ATable);
};
Shouldn't Contents[i] be set to NULL when I set the pointer p to NULL when it's pointing to it?Quote:
Originally Posted by matsp
Okay, trying the other suggestion, bbs.
So you mean the inner loop never happened because the first pointer is NULL? Check the address of ATable->Contents[i] the same way...Quote:
Originally Posted by P3nGu1N
Alright, well it keeps spitting out address:0 in the console, let me try the Contents[i] alone one for you.
p is not changing anything other than p.Quote:
Originally Posted by P3nGu1N
--
Mats
0 is not NULL, at least.Quote:
Originally Posted by P3nGu1N
Alright, just tried this:
and it keeps spitting out address (Contents[i]): ffe0b18, which is a valid memory address I'm guessing.Code:void DestroySymTab(struct SymTab *ATable) {
int i;
struct SymEntry *p;
for (i = 0; i < ATable->Size; i++) {
while(ATable->Contents[i] != NULL) {
/*p = (struct SymEntry*)ATable->Contents[i];*/
printf("address (Contents[i]): %p\n",ATable->Contents[i]); fflush(stdout);
/*while(p->Next != NULL){
p = p->Next;
}*/
free(p);
p = NULL;
}
}
free(ATable);
};
If that's not how I set p to point to Contents[i], is it supposed to be *p = (struct SymEntry*)ATable->Contents[i]; instead of just p = (struct SymEntry*)ATable->Contents[i]; ?
I'm seriously bad at this stuff, sorry if you feel like I'm wasting your time.
It can be, or at least 0 == NULL is true.Quote:
Originally Posted by MK27
I choose to waste it...anyway, the loop is infinite because you are basically sayingQuote:
Originally Posted by P3nGu1N
If ATable->Contents[i] was not NULL to start with (opening the loop) there is nothing in the loop that affects this pointer, as far as I can tell.Code:int x=5;
while (x==5) {
blah blah
}
It looks like you just want to iterate through the array, which would mean the while() inside the for() is superfluous. I think you should choose one or the other.
Well, I've known '\0' == NULL, but 0 == NULL? I miss programming in Java...Quote:
Originally Posted by laserlight
You should deduced that transitively since '\0' == 0 :)Quote:
Originally Posted by P3nGu1N
Maybe it would have been easier if you had missed programming in Java ;)Quote:
Originally Posted by P3nGu1N
lemme know if I was right about my last post
you made a very valid point... I'm really just trying to destroy each list item in an array of lists, which means I should only have to access each Contents[i] once provided it wasn't NULL to begin with, then set my pointer p to the beginning of the list only in that senario, which could be done with an if statement. Changed code to:Quote:
Originally Posted by MK27
and no more infinite loop. I just hope it does what I want.Code:void DestroySymTab(struct SymTab *ATable) {
int i;
struct SymEntry *p;
for (i = 0; i < ATable->Size; i++) {
if(ATable->Contents[i] != NULL) {
p = (struct SymEntry*)ATable->Contents[i];
while(p->Next != NULL){
p = p->Next;
}
free(p);
p = NULL;
}
}
free(ATable);
};
This code is absolutely broken, since it's not setting p to anything sensible, and thus tries to free "garbage".Quote:
Originally Posted by P3nGu1N
--
Mats
The sad thing is I've already created this program in Java last year. It's much easier for me to look at abstract data types like chain bucket hashing from an object oriented language.
Ok, so the new code only has one slight problem: You probably want to destroy EVERY element in the linked list, not just the last one.
--
Mats
What I'm trying to do is free the contents of a list of SymEntry s in the array associated with Contents of a SymTab. If I can't use a pointer, p to point to the address location where each SymEntry is, or if I'm doing it wrong, then how do I go about it?Quote:
Originally Posted by matsp
That's true, but how do I access the previous SymEntrys in the list after I destroy the last one? I was trying that outer while loop to destroy the last one, then the new last one, until the first SymEntry was freed, but that didn't work out so well for me.Quote:
Originally Posted by matsp
It is fine to use the code you have now - the code you posted that was printing stuff, you never set p to anything.
Your latest code looks OK, except that you only free the last pointer of the linked list - which is probably not what you want to do. Note that you will need a second pointer to hold the next pointer when you delete the current one.
--
Mats
I see a problem!
What you are doing there is looping thru until p->Next==NULL, then you free p it and set it to NULL. Meaning the last node, not all of them. To do what you want to do I think you will need a tmp pointer:Quote:
Originally Posted by P3nGu1N
Code:while(p->Next != NULL){
tmp=p;
p = p->Next;
free(tmp); tmp=NULL; /* the previous ones */
}
free(p); p=NULL; /* the last one */
Ok, so we're crossing posts. Just have another variable to hold the next pointer, and set the current to next when you have deleted current - that way, you don't have to worry about it.Quote:
Originally Posted by P3nGu1N
--
Mats
That's exactly what you want to do -- set p to point to each location you want to free and then free it. Currently you are setting p to nothing in particular, since you commented out any and every assignment involving p. Your original mistake was thinking that if you did something like this:Quote:
Originally Posted by P3nGu1N
that foo was later 5 (i.e., you assumed that contents[i] changed because you changed p). Assigning a pointer doesn't bind a reference, it just stores that address.Code:struct whatever *p = &foo;
p = 5;
A point to remember is that you can free in any order; i.e. you can do something like:
Code:while (next_to_free) is not NULL
to_be_freed = next_to_free;
next_to_free = next_to_free->next;
free(to_be_freed)
Or this:
I removed the "tmp = NULL" and "p = NULL", as if you us correct scopign for them, it shouldn't be necessary to set them to NULL to avoid invalid access.Code:while(p != NULL){
tmp=p;
p = p->Next;
free(tmp);
}
--
Mats
Yeah, I thought about that -- since you have no way of accessing the pointers now, it doesn't matter if they are NULL, and they have been freed.Quote:
Originally Posted by matsp
However, if you are just starting out with C I would say setting a pointer to NULL after a free() is a good habit to get into now, as in other circumstances it makes it easier to avoid a double free
Code:if (ptr) free (ptr);
; <- also, what is this??!!
that is a semicolon. (Yeah you probably knew that, but you can also end your methods with them. It doesn't matter whether it's there or not, just different style of coding).
Alright, I think you guys have helped me out enough where I can figure this out on my own (hopefully). If not, I'm sure I'll post again.
Actually I did not. This is my first year of C, after all.Quote:
Originally Posted by P3nGu1N