コンパイルできるのですが実行途中でエラーが

質問

#include<iostream>
#include<cstdlib>
using namespace std;

class stack{
char *stck;
int tos;
int size;
public:
stack(char c);
void push(char ch);
char *pop();
~stack();
};

stack::stack(char c){
tos=0;
stck=(char*)malloc(c);
size=c;
cout<<"生成スタック\n";
if(!stck){
cout<<"メモリ割り当てエラー\n";
exit(1);
}
}

void stack::push(char ch){
if(tos==size){
cout<<"スタックは一杯です\n";
return;
}
*stck=ch;
stck++;
tos++;
}

char *stack::pop(){
if(tos==0){
cout<<"スタックは空です\n";

return 0;
}
stck--;
tos--;
return stck;

}

stack::~stack(){
free(stck);
}

int main(){
stack s1(10),s2(10);
int i;
char *o;
s1.push('a');
s2.push('x');
s1.push('b');
s2.push('y');
s1.push('c');
s2.push('z');

for(i=0;i<5;i++){
o=s1.pop();
cout<<"s1をポップする"<<*o<<"\n";
}
for(i=0;i<5;i++){
o=s2.pop();
cout<<"ｓ２をポップする"<<*o<<"\n";
}
return 0;
}

なんですが、実行した所

生成スタック
生成スタック
s1をポップするc
s1をポップするb
s1をポップするa
スタックは空です

までで止まってしまいます。
たぶんreturn 0の戻り値の型がポインタだから止まってしまうのだろうと思います。
正しく実行するにはどうすればいいのでしょうか？
お願いします。

回答No.3

GOOD-Fr · ベストアンサー

理由はすでに回答があるとおりです。NULL ポインタを参照しているから。

まず最初に「までで止まってしまいます。」と書いていますが、質問するのならもう少し詳しく状況を書きましょう。最近の環境であれば NULL ポインタのアクセスを検出してエラーが出るのではないかと思います。
それから、戻り値として NULL を返してくる可能性がある関数を呼び出しているのですから、呼び出した側は戻り値が NULL であるかどうかを確認してからポインタを使わなくてはなりません。そのチェックが抜けています。

以下、本質的ではないお小言。

今回のプログラムはおそらく勉強用のサンプルなのだと思います。ですから、細かい部分にあれこれと突っ込むのは野暮だということは理解しています。が、それでも突っ込みたいところがいくつかあります。

1) stack クラスの仕様
stack クラスは push のときには char を引数として使っていますが、pop のときには char* を返しています。そして動作的には stack という名称どおり、スタック動作を行なっています。一般論としてスタックというのは、プッシュしたものがそのままポップされるものです。入るときと出るときで型が変わってしまうのでは誤解の元になりかねません。
2) pop の戻り値がポインタ
入るときと出るときで型が変わるのはいただけない、と言いましたが、それを気にしないとしても、いくつか気にするべきことがあります。ポインタを返していますから、そのポインタを使って stack クラスを呼び出した側は stack の中を直接操作できてしまいます。これでは内部構造を private にしている意味がありません。また、これはプログラマの責任ですが、このポインタを保持して「後で使おう」と思っても、スコープを抜けてしまったら stack クラスはデストラクタで破壊されます。当然、その領域をポインタで触ってはいけません。今回のプログラムでは気にしなくてよい点ですが、このふたつは頭に入れてプログラムをする必要があります。
3) クラス内での出力
「今回はサンプルで作ったから動作確認用なんです」と言うと思いますが、クラス内であれこれと出力するのはやめましょう。クラスは外部に対してインターフェースを提供するにとどめ、エラー出力や動作の確認については呼び出した側で行なうほうがいいでしょう。呼び出す側と呼び出される側の役割分担を適切に行なうのは、大きなプログラムをわかりやすく書くための基本中の基本です。
4) コンストラクタ stack::stack(char c)
このコンストラクタはなぜか char を引数にとっていて、しかも仮引数名が「c」となっています。これでは「最初にひとつ c という char を push するのかな？」と誤解されかねません。実際には、この仮引数はスタックサイズを現わしていて、しかも size という int に代入されています。名前もよくないですし、わざわざ char を引数として int にキャストさせるのは推奨できません。
5) 変数 tos と stck について
stack クラス内に tos と stck という二つの変数がありますが、この変数はひとつにまとめられます。同じ意味の変数がふたつあれば、そのふたつの変数の両方のつじつまを合わせる手間がかかるうえに、片方の変数だけを更新してしまうなどバグが入り込む可能性が増えます。不必要な変数は減らす努力をしましょう。
6) malloc と free
ダメとまではいいませんが、C++ でわざわざ malloc と free を使う理由はありません。new と delete を使いましょう。malloc 後にエラーチェックをしているのは感心ですし、確保したメモリを忘れずに free しているのもいいと思います。が、チェックしてそのまま exit してしまうのは、クラスとしては中途半端です。もちろん今回が「サンプル」であるのは理解していますので、わざわざ突っ込むのも野暮だとは思いますが、実用的なプログラムではこのへんをどう作りこむかが大切です。
7) STL について
今回はサンプルプログラムだと思うので、意味がない話ですが C++ では STL が用意されているのが普通です。プログラムでよく使われる構造については STL に用意されていますし、速度的にも有利です。また、これが大きなポイントですが、「自分でプログラムしないのだから、バグが入らない」のも重要です。余力があれば STL も勉強しましょう。

こんなところかな。
ああ、そうだ。「後はコピーコンストラクタ、代入演算子もお忘れなく」というのも大切ですね。

お礼

実はこのソースは入門書のサンプルを使って、自分なりに変更したものなんです。これからは自分の使用しているコンパイラと何処まで勉強したのかを書いておくようにします。

>「今回はサンプルで作ったから動作確認用なんです」と言うと思いますが

いや、沢山おしえてくれることはうれしいです。

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

回答No.5

S117

まじめにstackのクラスを考えるなら、空でないか判定するメンバ関数を足しましょう。
bool stack::isEmpty()
そして、popも戻り値の型を変更します。
char stack::pop()

使用する側ではisEmptyで先に空でないことを判定してからpopします。

さて、不正にpopが呼び出された場合の挙動ですが、素直に作るなら例外を投げるべきです。例外を使用することで、popの戻り値が必ずスタックからポップされた値になるため、うっかり不正な値を使用する危険がなくなります。またエラーの内容もより具体的で的確になります。

不正な呼び出しに対しては不正な値を返すのではなく、例外を返すのが（特別に事情がなければ）もっとも良い選択となるでしょう。

お礼

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

回答No.4

chie65536（@chie65535）

o=s1.pop();
cout<<"s1をポップする"<<*o<<"\n";
↓
o=s1.pop();
if (o!=0) {
cout<<"s1をポップする"<<*o<<"\n";
}

o=s2.pop();
cout<<"ｓ２をポップする"<<*o<<"\n";
↓
o=s2.pop();
if (o!=0) {
cout<<"ｓ２をポップする"<<*o<<"\n";
}

スタックが空でpop()が0を返して来たら、*oを参照してはいけない。

あと、他の回答にも書かれている通り「pop()がchar *を返すのは良くない」と思います。

これは「スタックが空の時、エラー値としてNULLを返し、そうじゃなければ成功したとしてポインタを返す」と言う、苦肉の策で「ポインタを返す」としているのでしょうね。

ですが、ここは「pop()はintを返す」とかにして「エラーの場合はpush出来ない値をintで返し、成功した場合はpushした値（文字）をintで返す」とした方が良いです。

例えば「成功の場合は-128～-1、0、1～127を返し、失敗の場合は256を返す」とか。

これなら「256をpushしても0がpopされ、成功した場合には絶対に256が返って来ない」ですから、成功とエラーが区別できます。

お礼

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

回答No.2

machongola

こんにちは。
　原因はNULLポインタの中を見てしまっているからです。
　void stack::push(char ch)　でchar型を一つずつスタックしているのですから、
　char* stack::pop()　でchar*を返す事自体が似合わないのでは？

　こうした方が良さそうです。
char stack::pop(){
//省略
//省略
stck--;
tos--;
return *stck;
}

　後はコピーコンストラクタ、代入演算子もお忘れなく。

お礼

それでも返せるんですか、思いつきませんでした。
ありがとうございます。

回答No.1

D-Matsu

0の代わりに空文字列("")でも返してやればいいのでは？
まぁ本当はいきなりcoutに投げるんではなく変数に受け取って中身チェックをすべきだろうと思いますが。

お礼

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