StackOverflow2013

Note that there are some explanatory texts on larger screens.

plurals

PO
primarykey
Id
12760480
data
AcceptedAnswerId
0
AnswerCount
0
ClosedDate
CommentCount
0
CommunityOwnedDate
CreationDate
2012-10-06T14:04:08.110
FavoriteCount
0
LastActivityDate
2012-10-06T14:04:08.110
LastEditDate
LastEditorUserId
0
OwnerUserId
1011995
ParentId
12759324
PostTypeId
2
Score
0
ViewCount
0
LastEditorDisplayName
text
Body
<pre><code>holder = (song_t *)malloc(sizeof(song_t)); /* snip */ /* else if */ holder = temp2->next->next; /* else */ holder = temp; temp2->next = temp->next; free(holder); </code></pre> When you assign <code>holder = temp2->next->next;</code> (incidentally, that's always <code>NULL</code> there), or <code>holder = temp;</code>, you're losing your reference to the <code>malloc</code>ed memory. Since you're not really using <code>holder</code> at all, the fix is to remove it from the function. In the first case, you're not using it in any way except to assign a complicated <code>NULL</code>, in the second case, removing <code>holder = temp;</code> and <code>free</code>ing <code>temp</code> is the correct way. There are a few more strange things and mistakes: <pre><code>song_t *DeleteSong(song_t *head, string key){ song_t *temp, *temp2, *holder; holder = (song_t *)malloc(sizeof(song_t)); // as said, remove holder completely temp = head; /* Find song with given title */ while(temp != NULL && strcasecmp(temp->title, key) != 0){ temp = temp->next; } if(temp == NULL){ newline; printf("Song not found."); newline; newline; } /* It's the very first in the list */ else if(temp == head){ if(temp->next == NULL){ /* It's even the only one */ temp->next = NULL; // This runs only if temp->next is already NULL free(temp); // Also free the members of temp, or you're leaking return NULL; } else{ head = temp->next; // You should now free temp and members, or you're leaking memory } } /* It's the last one in the list, but not the first */ else if(temp->next == NULL){ temp2 = head; /* Find the penultimate song */ while(temp2->next->next != NULL){ temp2 = temp2->next; } holder = temp2->next->next; temp2->next = NULL; // You should now free temp and members, or you're leaking memory } /* Neither first nor last */ else{ temp2 = head; /* Find song before */ while(temp2->next != temp){ temp2 = temp2->next; } holder = temp; temp2->next = temp->next; free(holder); } return head; } </code></pre> but apart from the leaks, it is correct, though unnecessarily complicated. <pre><code>song_t *DeleteSong(song_t *head, string key) { song_t *prev = NULL, curr = head; /* Find song and node before that */ while(curr != NULL && strcasecmp(curr->title, key) != 0) { prev = curr; curr = curr->next; } if (curr == NULL) { /* Not found */ newline; printf("Song not found."); newline; newline; } else if (prev == NULL) { /* It's the very first song in the list * so let head point to its successor * and free the song; it doesn't matter * if it's the last in the list */ head = curr->next; free(curr->title); // Probably, but not if title isn't malloced free(curr->artist); // Ditto free(curr->album); free(curr->genre); free(curr); } else { /* We have a predecessor, let that point * to the successor and free the song */ prev->next = curr->next; free(curr->title); // See above free(curr->artist); free(curr->album); free(curr->genre); free(curr); } return head; } </code></pre> Your <code>RemoveDuplicate</code> function on the other hand does not do what you intend. Apart from the distinction between the cases where the duplicate immediately follows the original and where it doesn't, for which I can see no reason, the assignments <code>temp = DeleteSong(temp,temp2->title);</code> resp. <code>temp2 = DeleteSong(temp2->next,temp2->title);</code> change what <code>temp</code> resp. <code>temp2</code> point to, but not where the respective predecessor in the list points. Let's illustrate the problem with a little ASCII art: <pre><code> temp temp2 | | v v song1->song2->song3->song4->song5->... </code></pre> where <code>song2</code> and <code>song3</code> are duplicates. Now in <code>temp = DeleteSong(temp,temp2->title);</code>, since the two songs are duplicates, the <code>head</code> argument of <code>DeleteSong</code> already matches, and in your version of <code>DeleteSong</code>, <code>head = temp->next; return head;</code> is all that is done, so the list is not changed at all, you just get <pre><code> temp temp2 | | v v song1->song2->song3->song4->... </code></pre> both pointers pointing to the same list node. In my version of <code>DeleteSong</code> that frees the node, <code>song1.next</code> would now point to <code>free</code>d memory, ouch. If the duplicate doesn't immediately follow the original, <code>temp2 = DeleteSong(temp2->next,temp2->title);</code> may not find a song with matching title, since the search starts after the known match, in which case the list is not modified at all, and <code>temp2</code> is just changed to point to the successor. If there is a song with matching title after that, the found duplicate is still not removed from the list, and the part after that may be changed, possibly leading to an unsound state. If <code>temp2</code> points to the penultimate node in the list, and the last node has a matching title, the last node is <code>free</code>d, but the <code>next</code> pointer of what was the duplicate isn't changed, so now it points to <code>free</code>d memory, you have a dangling pointer (that's a segfault waiting to happen). A further problem is that the criteria for removal in <code>DeleteSong</code> and <code>RemoveDuplicate</code> are different, in the former, you check only the title, in the latter also artist, album and genre, so using the former function in the latter risks deleting songs that should not be deleted (consider cover versions). When you want to delete a node from a singly linked list, you need a pointer to the preceding node to be able to change what that points to, or you'll create dangling pointers. I have given a standard way to do that above, you can basically copy that into the inner loop, but here it can never happen that we want to remove the first node, so it's a bit simpler: <pre><code>// void, since head is never changed, only duplicates after the original are removed void RemoveDuplicates(song_t *head) { song_t *orig = head, prev, curr; while(orig != NULL && orig->next != NULL) { prev = orig; curr = orig->next; while(curr != NULL) { // Find next song to remove while(curr != NULL && !meets_deletion_criteria(orig, curr)) { prev = curr; curr = curr->next; } // Now either curr is NULL, or it shall be deleted if (curr != NULL) { // Let the predecessor point to curr's successor prev->next = curr->next; clean_up(curr); // free all malloced members and the node itself curr = prev->next; } } orig = orig->next; } } </code></pre>
Tags
Title
singulars
PostAcceptedAnswerId
1. This table or related slice is empty.
PostParentId
1. POWhy does this keep on returning only one node of my singly linked list?
 singulars
 PostTypePostTypeId
 PTQuestion
PostTypePostTypeId
1. PTAnswer
UserLastEditorUserId
1. This table or related slice is empty.
UserOwnerUserId
1. USDaniel Fischer
plurals
PostLinksPostIdRelatedPostId
1. This table or related slice is empty.
PostLinksRelatedPostIdPostId
1. This table or related slice is empty.
PostsAcceptedAnswerId
1. This table or related slice is empty.
PostsParentIdCreationDate
1. This table or related slice is empty.
VotesPostIdCreationDate
1. This table or related slice is empty.
CommentsPostId
1. This table or related slice is empty.

Querying!

Guidance

A row detail

Detail views are divided into sections. All the information in the data section comes from columns in the selected row. The other sections display data from other, related rows.

Related data can be related in a to-one or a to-many fashion. Captions of data related in a to-many fashion link to a list view showing a filtered view of the table.

Try moving around until you find a non-empty to-many entry and click on the label to get to one. You can move back to the root by clicking on the database name in the header.