holder = (song_t *)malloc(sizeof(song_t));
/* snip */
/* else if */
holder = temp2->next->next;
/* else */
holder = temp;
temp2->next = temp->next;
free(holder);
またはを割り当てるとholder = temp2->next->next;(ちなみに、それは常にNULL存在します)、またはed メモリholder = temp;への参照が失われます。malloc実際にはまったく使用holderしていないため、修正は関数から削除することです。最初のケースでは、複雑な を割り当てる以外の方法では使用していませんNULL。2 番目のケースでは、削除holder = temp;してfreeing するtempのが正しい方法です。
さらにいくつかの奇妙なことと間違いがあります。
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;
}
しかし、リークは別として、不必要に複雑ではありますが、正しいです。
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;
}
一方、あなたのRemoveDuplicate機能はあなたが意図したことをしません。複製が元の直後に続く場合とそうでない場合の違いは別として、理由はわかりませんが、割り当てはそれぞれ異なりtemp = DeleteSong(temp,temp2->title);ます。temp2 = DeleteSong(temp2->next,temp2->title);何を変更しtempます。temp2を指しますが、リスト内のそれぞれの前任者が指している場所ではありません。少し ASCII アートで問題を説明しましょう:
temp temp2
| |
v v
song1->song2->song3->song4->song5->...
とは重複song2しています。では、2 つの曲が重複しているため、 の引数はすでに一致しており、あなたのバージョンのsong3では、すべてが完了しているため、リストはまったく変更されず、取得するだけです。temp = DeleteSong(temp,temp2->title);headDeleteSongDeleteSonghead = temp->next; return head;
temp temp2
| |
v v
song1->song2->song3->song4->...
両方のポインタが同じリスト ノードを指しています。私のバージョンでDeleteSongは、ノードを解放し、d メモリをsong1.next指すようになりました。free
複製が元の曲の直後に続いていない場合、検索は既知の一致の後temp2 = DeleteSong(temp2->next,temp2->title);に開始されるため、一致するタイトルの曲が見つからない可能性があります。この場合、リストはまったく変更されず、後続の曲を指すように変更されるだけです。その後にタイトルが一致する曲がある場合、見つかった重複がリストから削除されず、それ以降の部分が変更され、不健全な状態になる可能性があります。がリストの最後から 2 番目のノードを指し、最後のノードに一致するタイトルがある場合、最後のノードはdですが、重複していたもののポインターは変更されていないため、 d メモリを指すようになりました。ポインター (これは発生するのを待っているセグメンテーション違反です)。temp2temp2freenextfree
DeleteSongさらに問題として、とでは削除の基準RemoveDuplicateが異なり、前者ではタイトルのみ、後者ではアーティスト、アルバム、ジャンルもチェックするため、後者で前者の機能を使用すると、削除してはいけない曲まで削除してしまう危険性があります。 （カバーバージョンを検討してください）。
単一リンク リストからノードを削除する場合は、そのノードが指すものを変更できるように、前のノードへのポインターが必要です。そうしないと、ダングリング ポインターが作成されます。上記の標準的な方法を示しました。基本的にはそれを内側のループにコピーできますが、ここでは最初のノードを削除したいということは決してないので、少し簡単です。
// 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;
}
}