ポイントのコンストラクタをコピーする

Question

このコピーコンストラクタは正しいですか？

class GPSPoint 
{ 
    private: 
     double lat, lon, h; 
     char *label; 

    public: 
    GPSPoint (const GPSPoint &p) 
    { 
     if (this != &p) 
     { 
      lat = p.lat; 
      lon = p.lon; 
      h = p.h; 

      if (label != NULL) 
      { 
       delete [] label; 
       label = NULL; 
      } 

      if (p.label != NULL) 
      { 
       label = new char [ strlen(p.label) + 1 ]; 
       strcpy (label, p.label); 
      } 
     } 
    } 
} 

Answer 1

はい - 私はもともと誤って読みました。

しかし、あなたのためにメモリを管理し、コピーコンストラクタがはるかに簡単になるので（実際はこの場合は不要です）、ラベルとしてstd::stringを使用することをお勧めします。

Answer 2

少し冗長です。私はそれを再スタイルするだろう。それはすでに使用されている可能性があるかのようにあなたがNULLネスのためlabelをテストし、特にので

はもっと重要なのは、それは、より多くのコピーコンストラクタよりop=のように見えます。そして、初期化されていないので、すでにNULLである可能性は低いです... delete [] labelは、重大なエラーです。

これをあなたのop=に設定した場合、それは意味的に正しいと思います。

だから、あなたのコンストラクタを忘れて（とNULLにlabelを初期化する！）、コンストラクタをコピーしないとデストラクタ（それがop=を使用します）。

---

私はあなたが「std::stringを使用したくない」（任意の説得力のある根拠なし）あなたの前の質問で述べた知っているが、これは、なぜあなたが本当にすべきの完璧な例です。

Answer 3

コピーコンストラクタではなく、operator =の有効な実装を作成したと思います。 if (this != &p)は、オブジェクトがすでに存在する場合に意味があります。ラベルを削除する場合も同じです

Answer 4

あなたのクラスにポインタがある場合は、おそらく何か間違っています。

それとして再書き込みする方がよいでしょう：

class GPSPoint 
{ 
    private: 
     double  lat; 
     double  lon; 
     double  h; 
     std::string label; 

    public: 
    GPSPoint (GPSPoint const& copy) 
     : lat(copy.lat) 
     , lon(copy.lon) 
     , h(copy.h) 
     , label(copy.label) 
    {} 
    // You should also have an assignment operator 
    GPSPoint& operator=(GPSPoint copy) // Use Copy/Swap idum. 
    {          // Pass by value to get implicit copy 
     this->swap(copy);    // Now swap 
     return *this; 
    } 
    void swap(GPSPoint& copy) throw() 
    { 
     std::swap(lat, copy.lat); 
     std::swap(lon, copy.lon); 
     std::swap(h, copy.h); 
     std::swap(label,copy.label); 
    } 
}; 

は、今ではずっと簡単になります。

でもねえ、私たちは、コンパイラが生成コピーコンストラクタがあります忘れてしまった：
をだから今、あまりにも簡単になります。

完了

class GPSPoint 
{ 
    private: 
     double  lat; 
     double  lon; 
     double  h; 
     std::string label; 
}; 

を。よりシンプルな方が良いでしょう。

ポインタを絶対に保持する必要がある場合（ポインタが最適化されていないと思われるため）、ポインタを学習する必要がある場合（ポインターを使用する必要がありますが、使用しない場合は学習する必要があります）

class GPSPoint 
{ 
    private: 
     double  lat; 
     double  lon; 
     double  h; 
     char*  label; 

    public: 
    // Don't forget the constructor and destructor should initialize label 
    // Note the below code assumes that label is never NULL and always is a 
    // C-string that is NULL terminated (but that each copy creates 
    // and maintains its own version) 
    GPSPoint (GPSPoint const& copy) 
     : lat(copy.lat) 
     , lon(copy.lon) 
     , h(copy.h) 
     , label(new char[strlen(copy.label)+1]) 
    { 
     std::copy(copy.label, copy.label + strlen(label) + 1, label); 
    } 
    // You should also have an assignment operator 
    GPSPoint& operator=(GPSPoint copy) // Use Copy/Swap idum. 
    {          // Pass by value to get implicit copy 
     this->swap(copy);    // Now swap 
     return *this; 
    } 
    void swap(GPSPoint& copy) throw() 
    { 
     std::swap(lat, copy.lat); 
     std::swap(lon, copy.lon); 
     std::swap(h, copy.h); 
     std::swap(label,copy.label); 
    } 
}; 

Answer 5

要約：これは恐ろしい代入演算子ではありませんが、コピーコンストラクタとして壊れています。

まず、何も割り当てていないため、自己割り当てが発生する可能性はありません。 thisは、コンストラクタの開始時に初期化されていないブロブのメモリを指し、pはコピーしている完全に構築されたPointオブジェクトです。 2つは一致することはできません。初期チェックは時間の無駄です。

さらに詳しくは、labelがnullでないことを確認してください。私が言ったように、thisは初期化されていないメモリを指しています... labelこの時点でどのような値であっても、そのテストが合格するか失敗するかはわかりません。それがヌルでない場合、メモリのランダム部分delete[]に行きます。あなたが幸運であれば、これはすぐには失敗するでしょうが、そうする必要はありません。

スタイルに関しては、メンバを初期化するためのコンストラクタイニシャライザリストを優先します。

GPSPoint (const GPSPoint& copy) 
    : lat(copy.lat) 
    , lon(copy.lon) 
    , h(copy.h) 
    , label(0) 
{ 
    if(copy.label) { 
     label = new char[ strlen(copy.label) + 1 ]; 
     strcpy (label, copy.label); 
    } 
} 

正確性の点では、C文字列を取り除き、適切な文字列クラスを使用してください。次に、コピーコンストラクタを実装する必要はありません。

コピーコンストラクタを実装する場合は、必ずコピー代入演算子とデストラクタを実装してください。私はそれらが簡潔さのために除外されたと仮定しますが、そうでない場合、彼らは非常に重要です。

Answer 6

a1ex07がコメントで言うように、あなたのコードは、コピーコンストラクタよりもoperator=に入れたコードのほうがよく似ています。

まず、コピーコンストラクタが新しいオブジェクトを初期化しています。 if (this != &p)のようなチェックは、コピーコンストラクタではあまり意味がありません。あなたがコピーコンストラクタに渡しているポイントは、（新しいオブジェクトなので）そのポイントで初期化しているオブジェクトではないので、チェックは常にtrueになります。

また、はまだ初期化されていないため、if (label != NULL)などの作業は行われません。このチェックは、labelが偶然にNULLであるかどうかに応じて、ランダムな方法でtrueまたはfalseを返します。

私はこのようにそれを記述します。

GPSPoint(const GPSPoint& p) : lat(p.lat), lon(p.lon), h(p.h) { 
    if (p.label != NULL) { 
     label = new char[strlen(p.label) + 1]; 
     strcpy(label, p.label); 
    } 
    else { 
     label = NULL; 
    } 
} 

char*は良いアイデアだろうたぶん代わりにCスタイルのlabelstd::stringを作り、その後、あなたは純粋に初期化リストを使用して、コピーコンストラクタを書くことができます：

class GPSPoint { 
private: 
    double lat, lon, h; 
    std::string label; 

public: 
    // Copy constructor 
    GPSPoint(const GPSPoint& p) : lat(p.lat), lon(p.lon), h(p.h), label(p.label) { } 
}