C++ メンバー変数に自身のクラスを使う また、そのNULLチェック

Question

C++勉強中のものです。クラスについて、うまくわからないところがあるので質問させて下さい。

（以下のコードはC#ので書かれたものを、C++実現しようとして、書いたのでおそらく沢山ご指摘していただける部分があると思います。改善するべきところが多くあると思うので質問内容の外のご指摘も歓迎いたします。）

ノードオブジェクトに値、次のノード メンバーがあり、ShowList関数では自分の値をプリントし、次のノードを設定し、次のNodeがある限り、ループするというものです。　

Nodeクラスに、自身のNode型のメンバー　next を作りたいのですが、いろいろ検索した結果、ポインタかレファレンスを使わなくては実現できないと見つけ、レファレンスはその存在が保証されなければ壊れるので、避けるべきだと読みました。その結果、ポインタ、Node*　next に決定しました。　

質問１）一般にポインターはなるべく避けるべきと聞きますが、ポインターなしのより良い方法がありますか？

質問２） while(node != NULL)　次のノードが存在する限り、ループするというコードですが、これがうまく機能していないようです。つまり、threeまでしか、next を設定していないので、**1, 2, 3, 4 **と出力されると期待しているのですが、1, 2, 3, 4, 25, Segmentation fault:11 と出力されます。
何を改善するべきなのでしょう？

#include <iostream>

class Node{
public:
  Node(int v);
  int value;
  Node* next;
};

Node::Node(int v){
  this->value = v;
}

void showList(Node* node){

    while(node != NULL){
	std::cout << node->value << "\n";
	node = node->next;
    }
}

int main(){
  Node one(1);
  Node two(2);
  Node three(3);
  Node four(4);

  one.next = &two;
  two.next = &three;
  three.next = &four;

  showList(&one);

  return 0;
}

---

皆さん、ご回答ありがとうございました。色々、各解答、とても勉強になりました。一番勉強になったのは初期化の大切さです。以下、皆さんの、回答をもとに、試した、自分なりの答えです。

#include <iostream>

struct Node{
  Node(int val, Node* n);　　//初期化強制。ディフォルト無し。
  ~Node();
  int value;
  Node* next_address;
};

Node::Node(int val, Node* next_addr){
  this->value = val;
  this->next_address = next_addr;
}

void showList(Node* node){
    while(node != nullptr){
	std::cout << node->value << "\n";
	node = node->next_address;
    }
}


int main(){

  Node* one = new Node(1, nullptr);
  Node* two = new Node(2, nullptr);
  Node* three = new Node(3, nullptr);
  Node* four = new Node(4, nullptr);
  Node* five = new Node(5, nullptr);

  one->next_address = two;
  two->next_address = three;
  three->next_address = four;
  four->next_address = five;

  showList(one);


  // 借りたら、返す
  delete one;
  delete two;
  delete three;
  delete four;
  delete five;
  
  return 0;
}

Answer 1

こんにちは。

以下のコードはC#ので書かれたものを、C++実現しようとして

なるほど。C#とC++の相違点は数多いですが、個人的に最大の相違の１つはメモリ管理方式と考えています。
C++は「借りたら返す」です。返した後は使ってはいけません。間違って使ったら運が良ければ例外ですが、何事も無く動いて後で痛い目に合うケースもあります。

C#は「使い終わったら自動的に回収」です。使い終わらない限りいつまでも回収されず、使い終わっても直ぐには回収されません。そして、使い終わったら（null設定したら）使いようがなく、間違って使うと必ず例外が発生します。
一長一短ですが、C#のメモリ管理が楽な点はC#の生産性の高さに大きく貢献しています。

ポインタかレファレンスを使わなくては実現できないと見つけ、レファレンスはその存在が保証されなければ壊れるので、避けるべきだと読みました。その結果、ポインタ、Node*　next に決定しました。　

良い発想と思います。
基本的に参照はポインタの機能制限版です。ポインタの方が自由度が高いのですが、設計的に使わない機能は使えないように制限した方がバグは減ります。（間違って使わないので）
なのでポインタを使う必要がない時は参照を使った方が安全です。

質問１）一般にポインターはなるべく避けるべきと聞きますが、ポインターなしのより良い方法がありますか？

今回のコードでは生ポインタでも問題ないと思います。
「生ポインタはなるべくさけるべき」はメモリ・リークの可能性が高いからです。ポインタの指す領域をnew等で借りたら必ず返す必要があります。あちこちで借りまくるのできちんと返す設計は結構たいへんなため、バグの温床になりやすいです。なので「生ポインタはなるべくさけるべき」なのです。今回のコードはローカル変数を指しています。通常のローカル変数はその関数から抜ける時に自動的に開放されますからメモリ・リークの可能性はありません。

質問内容の外のご指摘も大歓迎いたします。

C++/C#共通の話で申し訳ないのですが、下記のインデントを嫌う人は多いと思います。

    while(node != NULL){
    std::cout << node->value << "\n";
    node = node->next;
    }

このwhile文の中にもう一つwhile文を入れて二重ループにすることを想定してみて下さい。ご理解頂けると思います。

また、Node* next;はprivateにした方が望ましいと思います。Node領域管理方式はNodeの内部実装になると思います。その方式を変更した時にNodeクラスを使っている外部のコードの修正が必要になるのは避けたいケースが多いです。privateにしておけば、外部のコードが「間違って」使う可能性はなくなりますから、管理方式を変更しても外部のコードへの影響はありえません。

なお、valueは微妙です。多少無理してもprivateにすべきとの意見もよく見かけますが、私はpublicも有りと思います。
こちらは有る種の構造体やコンテナ的な使い方と思います。例えば、valueの型を変更した場合、Nodeクラスを使っているコードも変更が必要になるケースが多いでしょう。この場合、手間をかけてprivate化するメリットが乏しいです。

Comments

[tf2014]

とても詳しい説明ありがとうございます。熟読し吟味、実験、失敗を繰り返したい思います。特に、借りたら返すの発想は面白そうです。qiitaの記事も飛ばし読みしてみましたが、時間をかけて、読んでいきます。

[tf2014]

コメントを読み直して思ったんですが、private, public / class , struct も色々考えさせられる部分なんですよね。いろいろ、読み漁ってみるとクラスと構造体の違いは、ディフォルトがパブリックかプライベートかが違うだけで後は同じと、かいてあり、そうすると、全て構造体にしたほうがすっきりするんじゃないかと考えたり、どの状況でどちらを使うべきなのか、迷います。　これは、これ自体でいろんなコメントを頂けると思うので、初心者なりの考えをまとめて別に質問させていただきます。

[tf2014]

メモリーリーク、ポインターの失敗も勉強の一環として、大失敗してみたいですけど。　グローバルでポインタをつくり、実験したいと思います。

[Chironian]

特に最近のC++は凄い発想の塊ですから得るものは非常に多いです。頑張って下さい。

Answer 2

コンストラクタが明らかに足りません。

Node::Node(int v){
  this->value = v;
}

これだけではnextに何も入っていない状態になってしまうため、

Node::Node(int v){
  this->value = v;
  next = NULL;
}

後はリストの場合はデストラクタが必ず必要です。
ないとプログラムが落ちますよ。
リストならstdの中にvectorやlistがあるのでそっちを
利用するのも手ですね。

Comments

[tf2014]

皆さんが書かれているように、C++は初期化が必要なのですね。勉強になります。

[stdio]

C++は初期化が必要なのは当たり前です。「知らないならc++をするな。」それがc++の掟。
という意見もありますよ。

[yumetodo]

それは言いすぎですが、未初期化の変数は作らないようにしたいですね。
constexpr関数にすれば自動的に未初期化の変数はコンパイルエラーになるのでおすすめです(違

[stdio]

言い過ぎかもしれませんが「c++は初期化と解放が命」なのでこれぐらい言わないとわかってもらえないのですよ。
全く!!これだから最近の若い者は!!初期化も知らんのか(24歳の若造)

[tf2014]

ありがとうございます。元のC#コードが Node one = new Node {Value = 1};　だったので、nextの初期化のことは考えてもいませんでした。　世に出回っている、各本、最初から「C++は初回化と開放が命」風に書いてもらえれば学びやすいんですけど。ディフォルト・コンストラクター（つまり、初期化なし）、コンストラクタ・オーバーロードで、適当なメンバーだけ初期化できるとか書いてあるので、どれでも大丈夫だと、C#と同じ感覚で考えていました。　いや、勉強になりました。（自分の不知を世界に公開してよかったです。）

[stdio]

C++の「初期化と解放は必須」は暗黙の了解ですからね。
本やサイトではC言語の知識がある前提で書くからね。

[fana]

（この書き方だと，「質問者のコードに対して」デストラクタが無いから落ちる，と指摘をしているように見えてしまいます）

[stdio]

確かにです。でも実際に落ちてるだろうから良いのではないでしょうか？

[Eki]

今回の質問者さんのケースでは、動的確保をしておらずローカル変数のアドレスを代入しているだけなのでデストラクタは不要ですね。実際に落ちてはいますが、今回はデストラクタとは無関係なので誤解を生む可能性はあるかなと見えました。
それでもリストに追加する要素がローカル変数の内容のみというのはあまり現実的ではないので、 (C# では要らない) メモリ解放処理が C++ では必須という意味でデストラクタは必要ですよね。 (そのときは逆にローカル変数のアドレスをリストに追加されると困りますが。)

[fana]

えっと，それはつまり【現象として「実際に落ちてる」のであれば，回答の記述としてはその原因側の話は正しくなくても別に良い】というようなスタンス(?)ということですか？

[fana]

> (そのときは逆にローカル変数のアドレスをリストに追加されると困りますが。)
nodeに好き勝手なアドレスが入れられ得るのに，デストラクタに解放処理を書く，というのは何だかおかしな話に感じてしまいます．
今回のケースで相手にしている実装は「publicなnextの値をいじくるのはNode型の利用側」という形ですから，nextに指定される領域を｛用意すること，必要ならば解放すること｝は全てNode型を利用する側の責任ではないかと思うのですが，どうなんでしょう？

Answer 3

// C++では"初期値は0"ルールが無い。nextを明示的に初期化すべし。
Node::Node(int v) : value(v), next(nullptr) {}

void showList(Node* node){
  for ( ; node != nullptr ; node = node->next ) {
    std::cout << node->value << "\n";
  }
}

Comments

[tf2014]

なるほど！「初期値は0"ルールが無い」勉強になります。　ありがとうございます。実験してみます。

[tf2014]

stdioさんのコメントを見た後、色々読みあさって、考えてみて、epistemeさんのコメントを読み直したら、Node::Node(int v) : value(v), next(nullptr) {}の便利さがわかりました。正直、昨日は意味をよく理解できていませんでした。「初期化が命」のため、nullptrに初期化。つまり、Node(int v, Node n)というコンストラクターを定義しなくても初期化はできるという便利ものですね。

Answer 4

#include <iostream>
#include <memory>
#include <functional>

class Node{
  int value;
  std::unique_ptr<Node> next;
public:
  Node(int v, Node* next_node = nullptr);
  ~Node();
  void eachValue(std::function<bool(int)> action);
};

Node::Node(int v, Node* next_node) : value(v), next(next_node){}
Node::~Node(){
  std::cout << "destruct(" << value << ")" << std::endl;
}
void Node::eachValue(std::function<bool(int)> action){
  if(action(value) && next)
    next->eachValue(action);
}

void showList(Node& node){
  node.eachValue([](int val){
    std::cout << val << std::endl;
    return true;
  });
}

int main(){
  Node one(1, new Node(2, new Node(3, new Node(4))));

  showList(one);

  return 0;
}

やってみたこと

• ポインタの代わりにスマートポインタ(unique_ptr)を使う
• valueおよびnextをprivateへ移行し、それに伴いeachValueやらコンストラクタやらを弄る
• 子ノードのデストラクタが呼び出される事を証明するためにデストラクタを定義

Comments

[tf2014]

ありがとうございます。スマートポインター、 eachValueなど、検索して勉強してみます。

[stdio]

突然すみません。
その解放ではNodeのnextのみを削除している様に見えるのですが...
解放する際に、後のリストが切り離されませんか？

[asm]

それを証明するためにデストラクタをわざわざくっつけてます

[stdio]

切り離されないのね。
質問者のやり方だと、解放時に後のリストの切り話を気にしながら実装しなきゃいけないから扱い間違えると脱線するからね～。その辺は質問者のやり方で実装したことある人間だから気になってました。

[tf2014]

率直に答えますと、stdioさんとasmさんの議論が私のC++の理解度を越えるものなので、返答できませんが、いろいろ失敗を繰り返し、試行錯誤していきたいと思います。

[stdio]

私が気にしてた事を省略すると、

リストはよく列車に例えられる。そして車両の一つ一つが今回でいう「next」。
列車は連結部を切り離すと、そこより後ろの車両は同然動力を失い、停車してしまう。
もしそれがプログラムの中で起こると、切り離した車両から後ろ(ここでいうnext->next)は行方不明になってしまって永遠(PCが起動している間)にメモリの海を彷徨うって訳。

[asm]

行方不明にはなりますが、
> 永遠(PCが起動している間)
は言いすぎですね。

現代的なOSの場合、プロセス終了時に割り当てたメモリは全解放されるシステムが一般的です。
起動しっぱなしのプログラムや特殊な環境でもない限り、そこまで神経質になる必要があるかは疑問です。

まぁ行儀悪いと言われればそれまでですが

[stdio]

そうですね。流石に言いすぎですね...
しかし、ゲーム開発経験があるとやたらと気になるのですよ。
ゲームにおいては、「プロセス終了時＝ゲームのプレイをやめた時」なので、しっかりと解放しないといけなかったりします。