マインスイーパーの上級がうまくいきません。

[かなたん]

この間の質問でウィンドウサイズを変更できるようになり、初級(マス：9×9　爆弾の数：10)だけでなく中級(マス：16×16　爆弾の数：40)もできました。
ですが、上級(マス：16×30　爆弾の数：99)がうまくいきません。
デバッグなしで開始してみてもなかなかウィンドウが出ず、デバッグ開始してどこで詰まっているのか調べてみることにしました。
すると、爆弾の配置部分に時間がかかっているようで、実行を一時停止させてみるとそのあたりで止まります。
そのまま実行させてみるとエラーが出ることもありましたが、今何度か試してみてもエラーにたどりつきません。
エラーの内容は
「マインスイーパー.exeの0x00f4c090でハンドルされていない例外が発生しました:0xC0000005:場所0xfdfdfdfdを読み込み中にアクセス違反が発生しました。」
で、矢印があった場所はマインスイーパーView.cppのOnDraw内で、マスがどういう状態かを判別するための部分です。

コード:

//一部抜粋
for(int x=0;x<pDoc->blocks.x;x++){ //CPoint blocks
	for(int y=0;y<pDoc->blocks.y;y++){
		if(pDoc->data[x][y]<0){ //int** data ここでエラーが出たらしい
			block_color.CreateSolidBrush(RGB(180,180,180)); //CBrush block_color
		}

初めのうちはmallocの失敗も疑いましたが、失敗してNULLが返ってきていないかどうかをif文で調べてみても、NULLが返ってきた様子はありません。
あのエラーが出たときにdataについて調べてみたのですが、data[x]は「評価できません」みたいなことを言われたように思います。
エラーが出ていたころと出なくなったころとで何か書き換えたような覚えはないのですが、今はなぜかなにもウィンドウが出ないままずっと実行中だと思って一時停止してみると爆弾を配置しているあたりで一時停止し、そのまま実行させてみても状況は進展しません。
今は爆弾配置部分に時間がかかっているようで全然うまくいきませんが、パソコンを再起動した直後は1度だけうまくいったこともありました。
ということはメモリが関係しているのでしょうか？
ガジェットのCPUメーターのRAMの部分を見ても50%以下(だいたい40%台)で、再起動する前も似たような値でした。
タスクマネージャーでプロセスを見てみると、メモリは828Kでしたが、CPUが25と重いように思いました。
(ネトゲで重いなーと思ってプロセスを見てみるとその値が出ていたりしました。)
16×30=480マスのどこかに合計99個の爆弾を配置するのって時間のかかることなのでしょうか？
また、あのようなエラーが出た原因は何なのでしょうか？

[softya（ソフト屋)]

dataはmallocした２次元配列でしょうか？　動作を見る限りは確保した範囲外をアクセスしている気配です。

>16×30=480マスのどこかに合計99個の爆弾を配置するのって時間のかかることなのでしょうか？
知覚できるほど時間はかかりませんので、分かるぐらい時間がかかるならバグがあると思います。

[usao]

>爆弾の配置部分に時間がかかっているようで、
ということがわかっているなら，(printfとリダイレクトとかでさくっと)ログを吐いてみてはどうでしょうか？

例えば，
爆弾の配置箇所が決定するたびに
　1個目 x y
みたいなのを出力．
xやyがうっかり範囲外になってないか等を見るとよいかもしれません．


>pDoc->data[x][y]
これが実は data[y][x] として参照すべき，ということだったりとかは…しませんよね？
あと，
>pDoc->blocks
が pDoc->Data[][]のサイズと合ってないとか．

[usao]

以上のことを確認してもわからない場合，

>dataはmallocした２次元配列でしょうか？
が真であるならば
dataの確保の部分とかを差し支えなければ提示されると良いかも？

[softya（ソフト屋)]

そうですね。もし２次元の動的配列なら、２次元の動的配列は確保・解放が面倒とかバグりやすいので余り使いません。
自分で一次元配列を確保してx,yから添字を計算したほうが問題が少ないからです。
　→　添字を求める計算式：x + y *横幅
ただ、この計算を毎回書くのはバグの元なので関数かマクロで隠蔽します。

[かなたん]

usaoさんの言うように配置がどのようになっているのか出力させて調べてみることにしました。
すると、配置に時間がかかる原因が分かりました。
爆弾を配置するときに何もないなら置いていいとしていたため、爆弾でなくヒントの数字が入っているとそこも置けないことにしていました。
そのために、今見てみると94個を置くのにずっと時間がかかり、93個置いてある状況で何もないマスは2つだけ。
480マスの中からその2マスを探すのに時間がかかっているようで。
そもそもその2つが見つけられたとしても置けるのは95個。
こんな調子ではどうやっても99個置けるはずがありません。
そんな状況をしばらく眺めていたところ、Windows版では上級はもっと空いているマスがあることを思い出し、ヒントの数字を入れているマスを爆弾で上書きしてもいいということに気が付きました。
そして爆弾が置ける条件をすでに爆弾が置かれていなかったらに修正したところ、何度実行させてみてもすんなりウィンドウが出てくれるようになりました。
こうやって気がつくまで配置部分のプログラムは問題ないと思っていたため、usaoさんに言われてログを出してみるまで気がつけませんでした。
自分のプログラムミスですいませんでした。
usaoさん・softya（ソフト屋)さんありがとうございました。

[Poco]

タスクマネージャーでプロセスを見てみると、メモリは828Kでしたが、CPUが25と重いように思いました。

お使いのPCはクアッドコアですか？
だとしたらCPU使用率が25%で固定になっているなら、コアのうち1つが無限ループ状態になっていると
考えればよいかと。

[softya（ソフト屋)]

それでバグは本当に治ったのでしょうか？
私には、まだ潜んでいる気がしてなりません。
※　x,yを大きな数字にだけにしてみるとバグが再現するかもしれません。
偶然発生しない状況で直ったと思い込むのはデバッグした事にはなりませんよ。

[かなたん]

お使いのPCはクアッドコアですか？
だとしたらCPU使用率が25%で固定になっているなら、コアのうち1つが無限ループ状態になっていると
考えればよいかと。

そういう意味があったりするんですね。
これから覚えておきます。
ちなみにコンピュータのプロパティーを見てみると以下のように書いてありました。
プロセッサ：Intel(R) Core(TM) i3 CPU M 330 @ 2.13GHz 2.13 Ghz

それでバグは本当に治ったのでしょうか？
私には、まだ潜んでいる気がしてなりません。
※　x,yを大きな数字にだけにしてみるとバグが再現するかもしれません。
偶然発生しない状況で直ったと思い込むのはデバッグした事にはなりませんよ。

私が設けている上限であるマス100×100の爆弾5000で試してみました。
(画面結構でかいな・・・w)
すると、以下のようなエラーになりました。
「マインスイーパー.exe の 0x782ac459 (mfc90ud.dll) でハンドルされていない例外が発生しました: 0xC0000005: 場所 0x0000000c を読み込み中にアクセス違反が発生しました。」

コード:

//開かれたafxwin1.cpp一部抜粋
_AFXWIN_INLINE BOOL CDC::Rectangle(LPCRECT lpRect)
	{ ASSERT(m_hDC != NULL); return ::Rectangle(m_hDC, lpRect->left, lpRect->top,
		lpRect->right, lpRect->bottom); } //この行に矢印が出ました。
_AFXWIN_INLINE BOOL CDC::RoundRect(int x1, int y1, int x2, int y2, int x3, int y3)
	{ ASSERT(m_hDC != NULL); return ::RoundRect(m_hDC, x1, y1, x2, y2, x3, y3); }

でもそのまま止めてもう一度実行すると、何も問題なく画面にはみ出るほどの大きいウィンドウがきちんと表示されました。
ということは、設定の変更でウィンドウサイズを変更したりするときは問題が起きるけど、初期設定でそうならうまく対処してくれるんだと思いました。
そのままプログラムを閉じずに設定をマス75×75の爆弾3000にして試してみました。
すると、今度は以下のようなエラーになりました。
「マインスイーパー.exe の 0x0025c1d0 でハンドルされていない例外が発生しました: 0xC0000005: 場所 0xfeeefeee を読み込み中にアクセス違反が発生しました。」

コード:

//矢印のあるマインスイーパーView.cpp一部抜粋
for(int x=0;x<pDoc->blocks.x;x++){
	for(int y=0;y<pDoc->blocks.y;y++){
		if(pDoc->data[x][y]<0){ //この行に矢印が出ました。
			block_color.CreateSolidBrush(RGB(180,180,180));
		}

自動変数に
pDoc->data 0x01651b80 int * *
0xfeeefeee int *
CXX0030: エラーです: 式を評価できません
pDoc->data[x] 0xfeeefeee int *
CXX0030: エラーです: 式を評価できません
とも表示されています。
でも、プログラムを止めてまた実行させてみると、まだ画面からはみ出るほど大きいウィンドウがなんのエラーもなく表示されました。
そのまま設定をマス50×50の爆弾1000に設定してみると、また同じエラーになりました。
でも、またプログラムを止めてまた実行させてみると、縦だけ画面からはみ出るくらい大きいウィンドウがなんのエラーもなく表示されました。
初期値だと問題ないのに設定変更だとエラーになるのは、PCの画面のサイズよりプログラムのウィンドウのサイズが大きいからですかね？
と思いつつ今度はマス25×50の爆弾500に設定してみましたが、それでも同じエラーになりました。
これも初期設定がそれだとエラーにはならないんですけどね。
そこでもしやと実は試していなかった設定で上級を選ぶというのをやってみると、これも同じエラーになりました。
なぜだろうと考えてみると、設定を変更させた後にマスのサイズに合わせてmallocしたりしているゲーム初期設定の関数を呼び出すのを忘れていました。
質問したときのエラーもそれが原因だと思います。
なぜなら設定で上級にしたときにあのようなエラーになりましたから。
でも同じような条件でやったはずの中級が問題なかったのが不思議ですが。
そのあとでmallocがうまくいっているかのif文を書き、設定を変更したらまたmallocされるつもりで設定変更後にメモリの解放をしておいた方がいいと思ってmallocに使った変数をfree関数に入れたりはしていましたが、結局実は設定変更の後ゲーム初期設定の関数を呼んでいなかったと。
でもこれで設定の最大値でもその他失敗した設定でも設定変更でエラーになることもなくなりましたから、今度こそ問題ないですよね？

[usao]

バグが解決したのかどうかはこちらからは基本的に知り得ないので，それはご自身で判断いていただくしかないかと．

デバッグ時は，現象面からの判断だけだと原因もよくわからないままになってしまうので
本当に治ったかどうか結局はっきりしなくなってしまいます．

自信を持って「俺はこのバグを潰した」と言い張るためには，
きちんとコードを追う他に，例えばログ等の証拠を収集して
「コードのこの箇所ががうっかり○○になっていた」→「その場合，××になってしまう」→「結果として△△になるため，そのようにバグっていたのだ」
「データ（ログとか）もそのことを物語っている」
みたく，起こっていた現象を理路整然と説明できることが必要かと思います．

オフトピック

ところで，本題と関係ないですが，ひょっとして爆弾の配置位置の決定方法が

コード:

for( int i=0; i<爆弾設置数;  )
{
    (x,y) = 乱数で決める;
    if( (x,y)に爆弾置けるなら )
    {
        (x,y)のマスに爆弾配置;
        ++i;
    }
}

みたくなっているのでしょうか．
フィールドの広さと爆弾個数の組み合わせ　が，この方法で十分なパターンしかないのであればあまり問題ないでしょうが，
「爆弾多すぎて開くべきマスは5%位しか残らない」みたいな設定でも
まともに（今回のように延々と置けない状態に陥らないように）動かしたいとかいうことがあるのであれば
効率よい方法に置き換えた方がよいかも？

[かなたん]

コード:

void CマインスイーパーDoc::GameSetting(){
	map=(int **)malloc(sizeof(int *)*blocks.x); //2次元配列のメモリをmallocで動的に確保
	if(map==NULL){
		AfxMessageBox(L"mapデータ領域を確保できませんでした。\nプログラムを終了します。");
		exit(1);
	}
	data=(int **)malloc(sizeof(int *)*blocks.x);
	if(data==NULL){
		AfxMessageBox(L"dataデータ領域を確保できませんでした。\nプログラムを終了します。");
		exit(1);
	}
	block=(CRect **)malloc(sizeof(CRect *)*blocks.x);
	if(block==NULL){
		AfxMessageBox(L"blockデータ領域を確保できませんでした。\nプログラムを終了します。");
		exit(1);
	}
	for(int x=0;x<blocks.x;x++){
		map[x]=(int *)malloc(sizeof(int)*blocks.y);
		if(map[x]==NULL){
			AfxMessageBox(L"mapデータ領域を確保できませんでした。\nプログラムを終了します。");
			exit(1);
		}
		data[x]=(int *)malloc(sizeof(int)*blocks.y);
		if(data[x]==NULL){
			AfxMessageBox(L"dataデータ領域を確保できませんでした。\nプログラムを終了します。");
			exit(1);
		}
		block[x]=(CRect *)malloc(sizeof(CRect)*blocks.y);
		if(block[x]==NULL){
			AfxMessageBox(L"blockデータ領域を確保できませんでした。\nプログラムを終了します。");
			exit(1);
		}
	}
	for(int x=0;x<blocks.x;x++){ //それぞれを初期化
		for(int y=0;y<blocks.y;y++){
			map[x][y]=0; //何も置かれていない
			data[x][y]=-1; //まだオープンされていない
			block[x][y].SetRect(x*20+10,y*20+40,x*20+30,y*20+60); //マスの位置に合わせる
		}
	}
	int bom=0; //配置した爆弾の数
	boms=bomsr; //配置させたい爆弾の数
	while(bom<boms){ //配置した数が配置させたい数より少なければ
		int x=rand()%blocks.x; //乱数で位置を決め、
		int y=rand()%blocks.y;
		while(map[x][y]==9){ //もしその位置に爆弾があるなら
			x=rand()%blocks.x; //何度でも新たに乱数で位置を決める
			y=rand()%blocks.y;
		}
		map[x][y]=9; //爆弾を配置
		for(int x2=x-1;x2<=x+1;x2++){ //周りにヒントとなる数字を配置させる
			for(int y2=y-1;y2<=y+1;y2++){
				if((x2>-1 && x2<blocks.x) && (y2>-1 && y2<blocks.y) && map[x2][y2]!=9){ //そこが画面内で爆弾が置かれているところでなければ
					map[x2][y2]++; //ヒントの数字を1増やす
				}
			}
		}
		bom++; //配置した爆弾の数を1増やす
	}
	marks=0; //印は1つも付いていない
	game=-1; //ゲーム中ではない
	settimer=false; //タイマーは作動させない
	timer=0; //タイマーを0で初期化
}

としていますが、それでは効率が悪いってことですかね？

[みけCAT]

usaoさんの擬似コードとほぼアルゴリズムが同じですね。
ほかの良さそうな方法としては、
1.置きたい数の爆弾をマップの上の方にかためて配置し、残りのますを空欄にする
2.マップのますをシャッフルする
3.ヒントの数字を計算する
という方法が考えられます。
例えば、9x9のますに爆弾を10個置く場合、最初に

コード:

*********
*........
.........
.........
.........
.........
.........
.........
.........

(*が爆弾、.が空き)
のように配置してからシャッフルします。
この方法を使用する場合、softya（ソフト屋)さんが言うような一次元配列を使う方がやりやすいかもしれません。

[softya（ソフト屋)]

せっかくｃ++なんですから、mallocは止めてnewにしましょう。
あとmap自体をクラス化して全メンバ関数において添字範囲外チェックを行った方が良いでしょう。
そうすれば、サイズ変更のための初期化を忘れて落ちるという事態をガードできます。

[かなたん]

ほかの良さそうな方法としては、
1.置きたい数の爆弾をマップの上の方にかためて配置し、残りのますを空欄にする
2.マップのますをシャッフルする
3.ヒントの数字を計算する
という方法が考えられます。

マス50×100の爆弾4999―つまり空きが1マスしかない状況でも重いということはなくすんなりウィンドウが出たので問題ないと思いましたが、参考にさせてもらいます。

せっかくｃ++なんですから、mallocは止めてnewにしましょう。
あとmap自体をクラス化して全メンバ関数において添字範囲外チェックを行った方が良いでしょう。
そうすれば、サイズ変更のための初期化を忘れて落ちるという事態をガードできます。

動的配列はmalloc・callocとvectorしか知らないもので・・・
mapをクラス化するという考えはありませんでしたが、今回のようなことになるなら今度からそういう風にしておいた方がいいのかもしれませんね。

[usao]

私が大昔にマインスイーパを作ったときは確か↓のような感じでやった気がします．

例えば，マップにマスがN個あり，各マスをindex値{0～N-1}で表せるとする．
このマップ上にb個の爆弾を配置する場合，

(1)マスのindexの配列を作る
　この配列の要素数はN-1．
　（最初にクリックされたマスには爆弾を置けないので．
　例えば，最初にクリックされたマスのindexが12番であれば，
　配列の内容は{ 0～11, 13～N-1 }のように12番を除いたものになる．）
(2)この配列をランダムにシャッフルする．
(3)結果の配列の [0]～[b-1]までのb個の要素が表すマスに爆弾を置く

という感じ．（index→ポインタでいい）
みけCATさんのとほぼ一緒かと思いきや，こっちは余計な配列使ってる分だけダメだな，うん．


>せっかくｃ++なんですから、mallocは止めてnewにしましょう。
malloc/free ではなく new/delete を使わなければならない　ことはあっても，その逆は無いから
new/deleteに統一した方が楽ですね．new側のが記述も楽だし．