C++の参照型の落とし穴：クラスメンバに参照型は使わないほうが良さそうだ

先日来、C++のデストラクタで、おかしな動きにぶち当たり「おかしい！バグか？」と大騒ぎした後、最終的に自分のミスに気がつきました。

タイトル通り、クラスにおける参照型に関する落とし穴です。

経緯とともにホントにお恥ずかしい限りですが、忘れた頃に同じことを繰り返しそうなので、恥を忍んで書いておきます。

f:id:takamints:20151227174609j:plain
photo credit: The Arm of Destruction via photopin (license)

プログラミング言語C++第4版

posted with amazlet at 15.12.28

ビャーネ・ストラウストラップ Bjarne Stroustrup
SBクリエイティブ
売り上げランキング: 5,351

Amazon.co.jpで詳細を見る

経緯はこう

クラスのインスタンスメンバに参照型を使ったシンプルなクラスを定義。
そのメンバをデストラクタで使っていたが正しく動作してないようだ。
「デストラクタが動いてないってどういうこと？！」と大騒ぎ（←最初の勘違い）
落ち着いて、確認用のコードを書いてみたら、デストラクタは動いていた
しかしデストラクタ内では、そのメンバの値が正しくない。コンストラクタでは正しかったが？
「おいおいデストラクタで参照型が使えないってどういうこと？！」と大騒ぎ（←２つめの勘違い）
ここまで全てVisual C++で動かしていたけど、G++では想定通りに動いていた。
「ほうらやっぱり Visual Studio おかしいぞ！」と大声出したら急に自信がなくなって、、、
じっくりコードを眺めてみたら、結局コンストラクタに単純ミスが。。。（自分か）

これが問題のクラス定義だ

スレッド間での排他処理のために、以下の様なクラスを定義したのです。

class MutexLocker {
    HANDLE& mutex;
public:
    MutexLocker(HANDLE mutex) : mutex(mutex) {
        WaitForSingleObject(mutex, INFINITE);
    }
    virtual ~MutexLocker() {
        ReleaseMutex(mutex);
    }
};

コンストラクタでミューテックスをロックして、そのスコープから抜けるときにデストラクタで開放するというものですね。（※ Visual Studio 2012以降では、スレッド間排他処理のために、CriticalSectionというクラスがあるようですので、そちらを使うほうが良いらしい。それ以前のバージョンではCRITICAL_SECTION構造体を使用。Mutexはプロセス間でも排他処理が可能なロックオブジェクトです）

ところが

以下のように使用しても、ミューテックスが開放されません。デストラクタ内ではmutexを正しく参照できなくて、どこの馬の骨ともしれないHANDLEを開放している。なぜだ。

class PacketLogger : Thread {
private:
    HANDLE mutex;
    std::deque<Packet*> packet_queue;
public:
    //   ・
    //   ・
    //   ・
    void AddPacket(Packet* p) {
        MutexLocker lock(mutex); //←ココ
        {
            packet_queue.push_back(p);
        }
    }
    //   ・
    //   ・
    //   ・
}

こうすればちゃんと動いていた

デストラクタで使用するメンバを参照でなく実体にすれば問題なかった

class MutexLocker {
    //HANDLE& mutex; //←参照でなく
    HANDLE mutex;    //←実体に
public:
    MutexLocker(HANDLE mutex) : mutex(mutex) {
        WaitForSingleObject(mutex, INFINITE);
    }
    virtual ~MutexLocker() {
        ReleaseMutex(mutex);
    }
};

確認コードを書いたはいいがむしろ勘違いを補強する

確認用に以下のコードを書いてみて、Visual Studio の3つのバージョン、2010、2013、2015で確認しても（2012は手元にない）、結果はすべて同じでした。DtorTestClassA のデストラクタでは、コンストラクタで出力した値を出力できない。最初に書きましたが、MinGW上のG++では、想定通りに動いていましたので、「VisualStudioでは、デストラクタで参照が壊れている」と考えちゃって大騒ぎ。

#include <iostream>

class DtorTestClassA {
    const int& number;
public:
    DtorTestClassA(int number) : number(number) {
        std::cerr << "construction DtorTestClassA #" << number << std::endl;
    }
    virtual ~DtorTestClassA() {
        std::cerr << "destruction DtorTestClassA #" << number << std::endl;
    }
};
class DtorTestClassB {
    const int number;
public:
    DtorTestClassB(int number) : number(number) {
        std::cerr << "construction DtorTestClassB #" << number << std::endl;
    }
    virtual ~DtorTestClassB() {
        std::cerr << "destruction DtorTestClassB #" << number << std::endl;
    }
};
int main(void) {
   DtorTestClassA a1(1);
   DtorTestClassB b1(1);
   {
       DtorTestClassA a2(2);
       DtorTestClassA b2(2);
   }
   for(int i = 0; i < 3; i++) {
       DtorTestClassA a3(i + 3);
       DtorTestClassA b3(i + 3);
   }
}

「Visual Studio のバグだろコレ！」って、んなわけないし。

ここで再び、じっくりコードを見なおしてみると、、、別のところがおかしいよと。

class MutexLocker {
    HANDLE& mutex;
public:
    MutexLocker(HANDLE mutex) : mutex(mutex) {
        WaitForSingleObject(mutex, INFINITE);
    }
    virtual ~MutexLocker() {
        ReleaseMutex(mutex);
    }
};

「コンストラクタおかしくないかい？」と。

コンストラクタの引数リストが、参照型になってないよと。

以下のようになっていないとダメじゃないかと？

class MutexLocker {
    HANDLE& mutex;
public:
    MutexLocker(HANDLE& mutex) : mutex(mutex) { //←ココだ
        WaitForSingleObject(mutex, INFINITE);
    }
    virtual ~MutexLocker() {
        ReleaseMutex(mutex);
    }
};

元のコードでは、メンバがコンストラクタのパラメータを参照していて、それってつまりコンストラクタの処理が終われば消滅しているオブジェクトですがな。そらあかんわ。

まとめと所感・教訓および反省文

検証大切

最初のコードを見た段階で気付ける人は幸せです。私は無理でしたが。気付けないのはまあ、アタマの程度の問題なので仕方がないけど、「ああ、こういうことなんだろうな？」という推測で進んだのがダメダメですな。お恥ずかしい。

人って基本的に、他者を疑うようになっていると思うけど、疑念を公言する前に各方面から確実に検証しなければ、こんな恥ずかしいことになるって事例です。大反省。

未来永劫クラスメンバで参照型は使わない

絶対にそうしなくちゃならないという理由なく、クラスメンバに参照型は使わないほうが良いかもしれないですね。

対象が構造体やクラスオブジェクトの場合はポインタで、完全になんの問題もなく代用可能ですから、そういう理由は今のところ見当たりません。

そもそもなんで最初に参照型にしたのかというと、明確に「そうしなくては」と思ったわけではなく「そのオブジェクトの生成と消滅には直接関わりたくない」という感覚的なものだった。

ところでG++の実装や如何に？

それから、むしろ、G++の動きのほうが怪しいという結果になったけど、もしかすると気を利かせて、こちらの意図に沿うように解釈してくれていたのかも？いや、単にスタックフレームの使い方が違うのかもしれませんね。

無視しないから警告出してよ

いやしかし、自分のミスを棚に上げてでも言っておきたいのだが、クラスメンバがコンストラクタのパラメータを参照しちゃっているのは明らかに間違いなんだから、せめてコンパイル時に警告出してほしいわ。静的解析したら警告なのかな？というか出てた？（←これがよくない）

銀の弾丸

プログラミングに関して、いろいろ書き残していければと思っております。