点的复制构造函数

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

如果你的类中有一个指针，那么你可能做错了什么。

最好将其重写为：

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 2

有点啰嗦；我会重新设计它的风格。

更重要的是，它看起来更像是 op= 而不是复制构造函数，特别是因为您测试 label 的 NULL 性，就好像它本来可以是已经用过。而且，由于它尚未初始化，它不太可能已经是 NULL...delete [] label 是一个严重错误。

但是如果你把它作为你的op=，那么我想这在语义上是正确的。

因此，不要忘记您的构造函数（并将 label 初始化为 NULL！）、复制构造函数（使其使用 op=）和析构函数。

---

我知道您在上一个问题中说过（没有任何令人信服的理由）您“不想使用 std::string”，但这是一个完美的例子，说明了为什么您确实应该使用。

Answer 3

根据我的理解，您创建了 operator = 的有效实现，而不是复制构造函数。
if (this != &p) 如果对象已经存在则有意义；删除标签也是如此

Answer 4

简而言之：不。这不是一个可怕的赋值运算符，但它作为复制构造函数被破坏了。

首先，不可能发生自分配，因为您没有分配任何东西。 this 指向构造函数开头处未初始化的内存块，而 p 是您正在复制的完全构造的 Point 对象。两者不能重合。所以最初的检查是浪费时间。

再往下，您检查以确保 label 不为空。正如我所说，this 指向未初始化的内存...此时label 可以是任何值，无法判断该测试是否会通过或失败。如果它确实产生不为空，您将删除[]内存的随机部分。如果幸运的话，这会立即失败，但并非必须如此。

就风格而言，更喜欢构造函数初始值设定项列表来初始化成员。

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 5

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

Answer 6

是的——我最初读错了。

不过，我仍然建议您使用 std::string 作为标签，因为它会为您管理内存，并且复制构造函数变得更加简单（事实上，对于这种情况是不必要的）。