複製點構造函數

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

這有點冗長;我會重新設計它。

更重要的是，它看起來更像是一個op=比複製構造函數，尤其是因爲你測試label的NULL ness好像它可能已被使用。而且，由於它沒有初始化，因此已不可能是NULL ... delete [] label是一個嚴重錯誤。

但是如果您將此設置爲您的op=，那麼我想這在語義上是正確的。因此，不要忘記你的構造函數（並初始化label至NULL！），複製構造函數（使其使用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對象。兩者不能重合。所以最初的檢查是浪費時間。

再下來，你檢查以確保label不爲空。正如我所說的，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尚未初始化。此檢查可能會以隨機方式返回true或false（具體取決於未初始化的label偶然爲NULL）。

我會寫這樣的：

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; 
    } 
} 

也許使label一個std::string，而不是C風格的char*將是一個更好的主意，那麼你可以純粹使用初始化列表寫你的拷貝構造函數：

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) { } 
}