自己参照型構造体のFree関数について

質問

///////////////////////////////////////
// 自己参照型構造体
///////////////////////////////////////
// 以下のソースで、free関数がうまく使えません。
// アドバイスを下さい。
// よろしくお願いします。
#include<iostream>
using namespace std;
struct data
{
int num;
data *next;
};
typedef struct data dat;

int main()
{
dat da;
dat *p1,*p2,*p3;
p1=&da;p2=&da;
for(int i=0;i<10;i++)
{
p1->next=(dat *)malloc(sizeof(dat));;
p1->num=i+1;
p1=p1->next;
}
int sum=0;
for(int i=0;i<10;i++)
{
sum+=p2->num;
p2=p2->next;
}
for(int i=0;i<10;i++)
{
p3=p2->next;
free(p2);
p2=p3;
}
cout << sum;
getchar();return 0;

回答No.5

mitoneko · ベストアンサー

#4補足宛
　気がつかれたようですね=^・。・^=。もとのソースがおかしかったのは、そこです。

　１０件目からの解放にどうしてもこだわられてるようですが。１件目から解放しても問題ないですよ。解放する前に、nextのアドレスさえ保管してしまえばよいわけです。mallocで確保した順番とfreeで解放する順番は対応している必要は「ありません。」ようするに、確保した物が最終的に一度だけ解放されればそれでよいのです。

　１件目から解放するとすると、#4補足に書かれたループは、少し簡単になります。

p1=dat.next;
for(int m=0;m<10;m++)
{
p2=p1->next; // p1を解放する前にnextを保存
free(p1);
p1=p2;
}

　こんな感じでしょうか。

　後は、スタイルの問題ですけど、このソース、全体としては、リストの初期化・リストの処理・リストの解放の３つの部分からなっています。
　これを各々、関数に分けると、全体の見通しがすっきりします。特にいろいろな処理に使い回しているp1・p2・p3が関数の中に封じ込められますので、この効果が大きいでしょうか。
　この程度の長さであれば、まぁ、なんとかなりますけど、もう少し、変数・処理が長くなると収拾つかなくなりますから、この例を書き直してみてどうなるか、ご自分で試してみられると良いでしょう。関数に分けるときには、datをグローバル変数にせずに、引数で渡すようにしてくださいね。

お礼

長々とお付き合いくださり、ありがとうございました。
お礼申し上げます。

補足

つまるところ、
p1=&da;
としたのは、
もともとそこは、
malloc確保ではなかったので、エラーになっていたということみたいですが、、、

ありがとうございました。

回答No.6

terra5

作り直すのがベストとは思いますが、一応現状の問題と対策を。

>私は、10件目のmallocから、freeしていけばいいと
>思っていますが、

この考え方は(適切では無いが)間違いではありません。
ですが、10件目のnextは11件目ですよね?
freeするのは9～1件目です。
意味的には p3 = p2->previous が正解になりますが、
previousはありません(^^;
previousを作る手もありますが、この場合本末転倒に近いので、
nextだけで実現する手を考えるのが普通です。
作った時と同じような手順をとりますが、
freeしてしまった後にはnextは参照できませんので、
一時保存する必要があります。

お礼

長々とお付き合いくださり、ありがとうございました。
お礼申し上げます。

補足

即席ですが、
こういうものが作れるようになりました。
皆様のおかげです。
/*
#include<iostream>
using namespace std;
struct data
{
int age;
char name[12];
data *next;
};
typedef struct data dat;

dat *nextpoint(dat *p);
void freedata(dat *p2,int cnt);
void display(dat *p3,int cnt);
int main()
{
dat da;
dat *p1;
p1=&da;
int select=0;
int cnt=0;
while(1)
{
printf("1:input 2:end 3:display -->");
scanf("%d",&select);
switch(select)
{
case 1:
printf("Name->");
scanf("%s",p1->name);
printf("Age ->");
scanf("%d",&p1->age);
p1=nextpoint(p1);
cnt++;
break;
case 2:
freedata(&da,cnt);
return 0;
break;
case 3:
display(&da,cnt);
break;
}
}
getchar();getchar();return 0;
}
dat *nextpoint(dat *p)
{
p->next=(dat *)malloc(sizeof(dat));
return p->next;
}
void freedata(dat *p2,int cnt)
{
dat *p3;
p2=p2->next;
for(int i=0;i<cnt;i++)
{
p3=p2->next;
free(p2);
p2=p3;
}
}
void display(dat *p3,int cnt)
{
for(int i=0;i<cnt;i++)
{
printf("name:%s age:%d\n",p3->name,p3->age);
p3=p3->next;
}
}
*/

回答No.4

はなおか じった（@Jitta）

C++で書き直すなら、malloc & freeではなく、new & deleteですよね？この２つはメモリ管理の仕方が違いますから、混ぜると危険です。また、C++なら、structと書いてもこれはデフォルトのアクセスレベルがpublicになるだけでclass宣言と変わりませんから、解放はクラスのデストラクタか、メソッドで行えばよいでしょう。

> このソースをどうしたいのかわかりません。
> 使い方がよくわからないです。

　『チェーンの使い方』と言うことでしょうか？
　たとえば、『malloc(sizeof(MyStruct) * 10)』とすると、MyStructが必要とする領域の１０倍を、一度に連続して確保しようとします。このとき、連続した空き領域がないと確保できず、nullが返ります。チェーンだと、１個ずつ確保するので、MyStruct分の空き領域が、連続していなくても１０個分あれば、確保できます。
　これはメモリのフラグメンテーションが関係する話なので、このように考えるといいかも。

○：空き
●：使用中
｜：わかりやすくする為の区切り
すべて空き：○○○○○○○○○○
３個確保：●●●｜○○○○○○○
３個確保：●●●｜●●●｜○○○○
３個解放：○○○｜●●●｜○○○○
ここで空きは７個あるが、７個確保しようとしても、連続した空きは３個と４個なので確保できない。しかし、１個を７回確保することは出来る。

> 初心者ですので、よろしくお願いします。

　初心者だろうが、仕事なら自分の仕事に責任持ちましょうよ。学生なら、将来の自分にたいして、自分で責任を持ちましょうよ。初心者であっても、初心者だからこそ、丸投げ（ソースください）はよくありません。“もらう癖”でなく、“理解する癖”をつけるべきです。

お礼

長々とお付き合いくださり、ありがとうございました。
お礼申し上げます。

補足

一応、考えてみました。
最後のFreeのところだけ、
for(int m=0;m<10;m++)
{
p1=&da;
for(int i=0;i<10-m;i++)
{
p1=p1->next;
}
free(p1);
}
こんな感じですが、
これでよろしいでしょうか？
>>丸投げ（ソースください）はよくありません。
はい、確かにそのとおりです。
行き詰まりのときは、丸投げします。
反省しています。
> 初心者ですので、よろしくお願いします。
なんか、これを書けば何とかしてもらえるという甘えです。
申し訳ありません。
皆様、ありがとうございました。

回答No.3

mitoneko

＃２の返事に対して、一つ質問しておきます。

＞私は、10件目のmallocから、freeしていけばいいと
＞思っていますが、

　の関係です。
　問題のstruct dataですが、今、ある要素があるとき、次の要素のアドレスを知ることは可能です。p1->nextですね。よって、先頭のdata構造体のアドレスが判れば、次の要素のアドレスを知ることはできます。（そして、すべての要素のアドレスが順番に取得できます。）
　では、ある要素のアドレスp1があるとき、前の要素のアドレスを求めることはできますか？

　１０件目から、さかのぼって領域を開放していくためには、これが求められる必要があります。３つめのループで、１０件目のp3から、９件目のp2をしることができないと、９件目の削除ができませんから。

お礼

長々とお付き合いくださり、ありがとうございました。
お礼申し上げます。

補足

回答ありがとうございます。

for(;;)を2回使用しました。
p1を先頭アドレスに指定しました。
10回ほどp1=p1->nextして、ラストのアドレスを指定し、
それをfreeしました。
もう一度、先頭アドレスを指定しました。
9回ほどp1=p1->nextして、アドレスを指定しました。
freeしました。

間違っていないと思うのですが、
これでいいのでしょうか？

回答No.2

mitoneko

根本的には・・・全面的に、書き直したいです(^_^;)
　これでは、回答にもアドバイスにもなりませんし、freeがうまくいかない理由だけ書いておきます。（どう修正するか、私が考えるとプログラムの原型が無くなってしまいます(^_^;)）

　これ、リンクリストですよね。
　一つめのループでリストの確保と、初期化。
　二つめのループで、そのリストの処理。

　ここまではいいです。
　ここまで終わった時点で、p1は、リストの最後の要素を、p2もリストの最後の要素を差しているはずです。

　とすると、３つめのループのリストのメモリー解放なんですが、この時点で、
　p3=p2->next;
　としているp2は、やっぱり、リストの最後の要素ですよね？
　ループの一回目のfreeは、最後の要素の解放。では、最後の要素の次の要素と指定されたp3にはいったい何が？
　かくして、ループの２回目は、もう何をやっているのかわかりません。最後の要素のnextの初期化はされていないのと思うので、動作は未定義です。
　では、p3を最初に初期化しておいて（p2の使い方と同じようにです。）やるとすると・・・最初の要素が
dat da;
　で定義されてますので、単純にすると、free関数は使えません。(freeでは、mallocで確保したメモリーしか解放できませんから。）これに注意しておけば、後は単純です。

　とこれで、後は、問題の所を直せばｏｋと思います。が・・・アドバイスをと言われると、基本的には、やっぱり、プログラムの構造をなおしたくなります。どう直すかは、#1さんがあげられてますから、私は、ふれないことにします。

お礼

長々とお付き合いくださり、ありがとうございました。
お礼申し上げます。

補足

回答ありがとうございます。

malloc と free の
処理の仕方が全然違うのでしたら、
原型なくなってもいいので、
プログラムの構造を変更した
サンプルソースを教えてください。
お願いいたします。

私は、10件目のmallocから、freeしていけばいいと
思っていますが、
p1=p1->next の時点で何だかわからなくなりました。
それから、入門書も関数処理が書いてありましたが、
よくわかりません。文字列の処理でしたが、
複雑でしたので、よくわかりませんでした。
int ならわかると思い、mallocで領域確保したはいいですが、freeが使えません。

回答No.1

toysmith

このソースをどうしたいですか？

１．関数分割せずに１関数で全て処理したい。
２．適度に関数分割しても良いが、再起は使いたくない。
３．メイン、構造体確保関数、構造体開放関数にわける。再起しても良い。

おすすめは３なんですけど…

あと、べたべたにCで書いてる割にはiostreamだったり、coutだったりするんですが、完全にC++で仕様から考え直すって言う選択肢もあります。

お礼

長々とお付き合いくださり、ありがとうございました。
お礼申し上げます。

補足

回答ありがとうございます。
3番目のようにしたいですが、
いきなり処理を分けてしまうと、よくわからなくなります。再起というのは、意味がよくわからないです。

free関数の使い方がよくわかっていません。
すみません。

Ｃ＋＋に直したいです。

このソースをどうしたいのかわかりません。
使い方がよくわからないです。
データの件数が不定のときに、使うのかな程度にしか
理解していません。

すみません、最後の二行についてもよくわからないです。
stdio.h , printf("",); の方がよいということでしょうか？

初心者ですので、よろしくお願いします。