Копировать конструктор для точки

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 ness, как если бы он мог уже использоваться. И, поскольку он не инициализирован, он вряд ли будет NULL уже ... delete [] label является критической ошибкой.

Но , если бы вы сделали это своим op=, тогда, я думаю, это было бы семантически правильно.

Так что не забывайте свой конструктор (и инициализируйте label в NULL!), Копируйте конструктор (используйте его op=) и деструктор.

---

Я знаю, что вы заявили в своем предыдущем вопросе (без какого-либо убедительного обоснования), что вы "не хотите использовать std::string", но это прекрасный пример того, почему вы действительно должны.

Answer 3

Короче говоря: нет.Это не ужасный оператор присваивания, но он не работает как конструктор копирования.

Во-первых, нет возможности для самопредставления, потому что вы ничего не присваиваете.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 4

В моем понимании вы создали правильную реализацию operator =, а не конструктор копирования. if (this != &p) имеет смысл, если объект уже существует; то же самое для удаления метки

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 для метки, так как она будет управлять памятью для вас, и конструктор копирования станет намного проще (на самом деле, в этом случае нет необходимости).