リストでつまづいています

[オーシロ]

Ｃ言語でリストを学んだので、Ｃ++のクラスを使ってそれっぽく作れないかと思い、
以下の様なプログラムを組みました。

#include <stdio.h>

//基本データ。ここを改造してキャラクターの基本クラスを作る
class Data{
public:
　　　　//暫定的に配置するデータ。１つ前のノードより１大きい数が入る
　　　　int i;

　　　　//コンストラクタ
　　　　Data(int j){
　　　　　　　　i = j + 1;
　　　　}
　　　　//デストラクタ
　　　　~Data(){
　　　　　　　　printf("　　%dDataを削除します\n", i);
　　　　}

　　　　//iの中身を確認する
　　　　void nprint(){
　　　　　　　　printf("　　　　　%dDataの内容は%d\n", i, i);
　　　　}
};

//リストの１つ１つのデータ
class Node{
public:
　　　　//データのアドレス
　　　　Data* here;
　　　　//１つ前のノードのアドレス
　　　　Node* prev;
　　　　//１つ先のノードのアドレス
　　　　Node* next;

　　　　//コンストラクタ。指定されたとおりにくっつけます
　　　　Node(Data* h, Node* p, Node* n){
　　　　　　　　here = h;
　　　　　　　　prev = p;
　　　　　　　　next = n;
　　　　}
　　　　//デストラクタ。
　　　　~Node(){
　　　　　　　　printf("　%dNodeを削除します\n", here->i);
　　　　　　　　delete here;
　　　　　　　　prev->next = next;
　　　　　　　　next->prev = prev;
　　　　}
};

//リストそのもの。
class List{
public:
　　　　//first。先頭。
　　　　Node* fst;
　　　　//ダミー
　　　　Node* dummy;
　　　　//要素数。
　　　　int num;

　　　　//コンストラクタ
　　　　List(){
　　　　　　　　//ダミーの実体？生成
　　　　　　　　dummy = new Node(new Data(-1), NULL, NULL);
　　　　　　　　//先頭のアドレスをダミーに。
　　　　　　　　fst = dummy;
　　　　　　　　//要素数は０に。（ダミーは数えない）
　　　　　　　　num = 0;
　　　　}
　　　　//デストラクタ
　　　　~List(){
　　　　　　　　//削除するアドレスを２番目にセッティング
　　　　　　　　Node* dcur = fst->next;
　　　　　　　　
　　　　　　　　printf("listを削除します\n");

　　　　　　　　//削除部分。入れ替えなんかの部分はＮｏｄｅクラスの
　　　　　　　　//デストラクタで済ませている…つもり
　　　　　　　　while(dcur){
　　　　　　　　　　　　Node* next = dcur->next;
　　　　　　　　　　　　delete dcur;
　　　　　　　　　　　　dcur = next;
　　　　　　　　}
　　　　}

　　　　//要素追加
　　　　void add(){
　　　　　　　　//先端まで移動
　　　　　　　　Node* l = fst;
　　　　　　　　while(l->next != NULL) l = l->next;
　　　　　　　　
　　　　　　　　//先端に新しいデータをくっつける
　　　　　　　　l->next = new Node(new Data(l->here->i), l, NULL);
　　　　　　　　//要素数を加える
　　　　　　　　num++;
　　　　}
　　　　//要素削除
　　　　void dlt(int n){
　　　　　　　　//指定された場所まで移動
　　　　　　　　Node* l = fst;
　　　　　　　　if(n <= 0 || n > num) return;
　　　　　　　　for(;n!=0;n--){
　　　　　　　　　　　　if(l-> next == NULL) return;
　　　　　　　　　　　　l = l->next;
　　　　　　　　}
　　　　　　　　//その場所を削除
　　　　　　　　delete l;
　　　　　　　　//要素数を減らす
　　　　　　　　num--;
　　　　}
　　　　//中身の参照
　　　　void move(){
　　　　　　　　Node* l = fst->next;
　　　　　　　　while(l != NULL){
　　　　　　　　　　　　l->here->nprint();
　　　　　　　　　　　　l = l->next;
　　　　　　　　}
　　　　}
};

int main(){
　　　　List lst;
　　　　lst.add();
　　　　lst.add();
　　　　lst.add();
　　　　lst.add();
　　　　lst.add();
　　　　lst.dlt(3);
　　　　lst.move();
　　　　return 0;
}

その結果は

　3Nodeを削除します
　　3Dataを削除します
　　　　　1Dataの内容は1
　　　　　2Dataの内容は2
　　　　　4Dataの内容は4
　　　　　5Dataの内容は5
listを削除します
　1Nodeを削除します
　　1Dataを削除します
　2Nodeを削除します
　　2Dataを削除します
　4Nodeを削除します
　　4Dataを削除します
　5Nodeを削除します
　　5Dataを削除します

としっかり出たのですが、その後何故か強制終了されてしまいました。

原因はNodeクラスのデストラクタあたりにありそうなのですが、どのように解決すればよいかがわかりません。
何かヒントをもらえないでしょうか？

[ookami]

コピーコンストラクタが見当たらないのは、関係ありますかね？

[オーシロ]

>>ookamiさん

コピーコンストラクタという存在をまず知りませんでした。
まだ勉強が足りていませんね…すみません。

というわけで、
//コピーコンストラクタ
List(const List& obj){
　　　　dummy = new Node(NULL, NULL, NULL);
　　　　*dummy = *obj.dummy;
}
というのを追加してみたのですが、また強制終了されてしまいました。

となると、dummy = new Node(new Data(-1), NULL, NULL);という部分の、
「new Data(-1)」という部分が強制終了を引き起こしている元なのだと思うのですが、
ここをどうすれば良いのかがいまいちよく分かりません…。

[h2so5]

とりあえずヒントを。

お察しの通り、Nodeクラスのデストラクタに問題があります。

オーシロさんのコードでは、削除されるNodeに必ず次のNodeがあることが前提になっています。
削除されるNodeがリストの一番最後、つまり次のNodeがない場合が考慮されていません。

[ookami]

「コピーコンストラクタ」と「operator=」は、自分で定義しないとコンパイラが勝手に
適当なものを作ってくれてしまうため、
特にメンバにポインタがある場合には、予期しない動きをすることが多いです。

ただ、今回のケースでそれが原因なのか、実は私、確証なかったりします。
どう見ても知ったかぶりです。本当にry

[オーシロ]

>>h2so5さん

なるほど、nextがNULLになっているNodeにも参照していたから強制終了が起こったわけですね。

というわけで、以下のように変更してみたところ、
　　　　//デストラクタ。
　　　　~Node(){
　　　　　　　　printf("　%dNodeを削除します\n", here->i);
　　　　　　　　delete here;
　　　　　　　　if(next != NULL){
　　　　　　　　　　　　prev->next = next;
　　　　　　　　　　　　next->prev = prev;
　　　　　　　　}
　　　　　　　　else prev->next = NULL;
　　　　}
無事、強制終了されることなくプログラムを終了させることができました！


>>ookamiさん

今回はどうもそれとは違う部分が強制終了させていたようです。
ですが、とても参考になりました。もっと勉強しなくてはいけないと感じました。
ありがとうございました。


ookamiさん、h2so5さん、ありがとうございました！

[けえぼお]

~Node() の prev->next = next; にも問題があると思います。（そのままでも強制終了はされませんが。）
~List() の delete dcur; で既に削除された要素を参照してます。

自分がリストクラスを作ってみたときは、Listクラス内に
prev->next = next;
next->prev = prev;
に当たる内容を書いたような気がします。

素人の意見ですがご参考までに。。。

[gyz]

解決とされたようですが
コードを見て気づいた点があったので
レスさせていただきます。

Listクラスのデストラクタでfst(dummy)をdeleteしてないので
このままではメモリリークが発生します
デストラクタでちゃんとdeleteするか
fst(dummy)をNode*ではなくNodeにする必要があると思います

あと、Listクラスのdummyが意味なくないですか？
dummy = new Nodeして、そのアドレスをそのままfstに渡してるので
結局fst = new Nodeするのと変わらない処理になってると思います
dummy->fst->それ以降のリスト...というのを作る意図なら

dummy = new Node(略);
fst = new Node(new Data(dummy->here->i), &dummy, NULL);
dummy->next = fst;

こんな感じになるんではないかと思います