2011-01-15 30 views
13

C++でコピーコンストラクタとコピー代入演算子について読んだ後、簡単な例を作成しようとしました。以下のスニペットは明らかに機能しますが、私はコピーコンストラクタとコピー代入演算子を正しく実装しているかどうかはわかりません。関連する概念を理解するのに間違いや改善がある場合や、より良い例がある場合は、指摘してください。C++:コピーコンストラクタとコピー代入演算子の実装

class Foobase 
{ 
    int bInt; 

public: 
    Foobase() {} 

    Foobase(int b) { bInt = b;} 

    int GetValue() { return bInt;} 

    int SetValue(const int& val) { bInt = val; } 
}; 


class Foobar 
{ 
    int var;  
    Foobase *base;  

public: 
    Foobar(){} 

    Foobar(int v) 
    { 
     var = v;   
     base = new Foobase(v * -1); 

    } 

    //Copy constructor 
    Foobar(const Foobar& foo) 
    {  
     var = foo.var; 
     base = new Foobase(foo.GetBaseValue()); 
    } 

    //Copy assignemnt operator 
    Foobar& operator= (const Foobar& other) 
    { 
     if (this != &other) // prevent self-assignment 
     { 
      var = other.var; 
      base = new Foobase(other.GetBaseValue()); 

     } 
     return *this; 
    } 

    ~Foobar() 
    { 
     delete base; 
    } 

    void SetValue(int val) 
    { 
     var = val; 
    } 

    void SetBaseValue(const int& val) 
    { 
     base->SetValue(val); 
    } 

    int GetBaseValue() const 
    { 
     return(base->GetValue()); 
    } 

    void Print() 
    { 
     cout<<"Foobar Value: "<<var<<endl; 
     cout<<"Foobase Value: "<<base->GetValue()<<endl; 

    } 

}; 

int main() 
{ 
    Foobar f(10);  
    Foobar g(f); //calls copy constructor 
    Foobar h = f; //calls copy constructor 

    Foobar i; 
    i = f; 

    f.SetBaseValue(12); 
    f.SetValue(2);  

    Foobar j = f = z; //copy constructor for j but assignment operator for f 

    z.SetBaseValue(777); 
    z.SetValue(77); 

    return 1; 
} 

答えて

12

コピー代入演算子が正しく実装されていません。オブジェクトがbaseが指すオブジェクトをリークするように割り当てられています。

あなたのデフォルトコンストラクタも間違っています:それはbasevar初期化されていないの両方を残して、あなたがdelete base;を呼び出すとき、有効とデストラクタであるいずれかのかどうかを知る方法はありません、悪いことが起こります。

コピーコンストラクタとコピー代入演算子を実装し、正しく行ったことを知る最も簡単な方法は、the Copy-and-Swap idiomを使用することです。

+1

+1良い点コピーとスワップのイディオムは、オブジェクトが一貫性のある状態を保つことを保証する例外セーフ実装を行うために不可欠です。 – jdehaan

+0

デストラクタ '〜Foobar()'によってメモリが解放されませんか? – blitzkriegz

+0

@Mahatma: '〜Foobar()'は呼び出されません。オブジェクトは決して破壊されず、割り当てられます。 –

2

Foobarのみカスタムコピーコンストラクタ、代入演算子、およびデストラクタが必要です。 Foobaseは、コンパイラが提供するデフォルトの動作で十分です。

Foobarの場合、代入演算子にリークがあります。それを割り当てる前にオブジェクトを解放することで簡単に修正できます。十分に良いはずです。しかしFoobarポインタメンバーを追加した場合は、状況が複雑になることがあります。今度は、2番目のポインタの割り当て中に例外が発生した場合、破損やリークを避けるために、割り当てた最初のポインタを適切にクリーンアップする必要があります。また、ポインタメンバーを追加すると、多項式の方法より複雑になります。

代わりに、代入演算子をコピーコンストラクタの観点から実装します。次に、非スロースワップ関数の観点からコピーコンストラクタを実装する必要があります。詳細はCopy &スワップイディオムを参照してください。

また、デフォルトのコンストラクタFoobarでは、メンバーをデフォルトで初期化しません。それはユーザーが期待するものではないため、悪いことです。メンバポインタは任意のアドレスを指し、intは任意の値をとる。今あなたがコンストラクタを作成したオブジェクトを使用する場合、あなたは非常に未定義の動作の土地の近くです。

+0

元のメモリを解放するために新しい割り当ての前に 'delete base;'を実行すると、 'double free or corruption'エラーが出ます。私はイディオムを調べます。私は空のデフォルトコンストラクタの問題について最後の部分を得ていませんでした。私は 'var = SOMEVALUE;'と 'base = NULL;'を置くべきでしょうか?私の例では、私はパラメータ化されたコンストラクタを使用しているからです。 – blitzkriegz

+0

* new-expression *に括弧がないと、コンストラクタのない型に対してのみ違いがあります。この型はどれもありません。少なくとも1つのユーザ定義コンストラクタを持つ型の* default-initialization *は、デフォルトコンストラクタの呼び出しを生成します。 –

+0

デフォルトのコンストラクタは、* ctor-initializer *を持たないことで* mem-initializer-id *には何も記述されておらず、* default-initialize * * この場合)。しかし、 'int'のデフォルトの初期化は状態を未定義にします。 –

0

私はあなたのための非常にシンプルなパッチがあります。

class Foobar 
{ 
    int var;  
    std::unique_ptr<FooBase> base; 

... 

あなたが始める必要があります。

一番下の行は次のようになります。 (...あなたがよく知っている)(専門家はポイント2を参照)

  • コード内でdeleteを呼び出さないでください、あなたのコード内でdelete

    1. 呼ばないでください。
  • +0

    コンパイラに付属しているC++ライブラリの年齢によっては、 'std :: tr1 :: unique_ptr' ... –

    関連する問題