Copy constructor for point

Is this copy constructor correct?

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

If you have a pointer in your class you are probably doing something wrong.

It would be better to re-write it as:

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

Now that looks a lot simpler.

But hey we forgot there are compiler generated copy constructors:
So it now simplifies too:

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

Done. The simpler the better.

If you must absolutely keep the pointer (because you think it is an optimization (its not), or you need to learn pointers (you do, but you need to learn when not to use them)).

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

It's a bit verbose; I'd re-style it.

More importantly, it looks more like an op= than a copy constructor, especially because you test label for NULLness as if it could have been used already. And, since it's not initialised, it's unlikely to be NULL already... delete [] label is then a critical error.

But if you made this your op=, then I guess that would be semantically correct.

So then don't forget your constructor (and initialise label to NULL!), copy constructor (make it use op=) and destructor.

I know you stated in your previous question (without any convincing rationale) that you "don't want to use std::string", but this is a perfect example of why you really should.

In my understanding you created a valid implementation of operator =, not a copy constructor. if (this != &p) makes sense if the object already exists ; the same for deleting label

In short: no. That's not a horrible assignment operator, but it is broken as a copy constructor.

First, there is no possible way for self-assignment to occur, because you are not assigning anything. this points to an uninitialized blob of memory at the start of your constructor, while p is a fully constructed Point object that you are copying. The two can not coincide. So the initial check is a waste of time.

Further down, you check to ensure label is not null. As I said, this points to uninitialized memory... label can be any value at this point, there is no telling whether or not that test will pass or fail. If it does yield not null, you are going to delete[] a random part of memory. If you are lucky, this will fail immediately, but it doesn't have to.

In terms of style, prefer constructor initializer lists for initializing members.

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

In terms of correctness, get rid of the c-string, and use a proper string class. Then, you won't need to implement a copy constructor at all.

No matter what, if you implement a copy constructor make sure you implement a copy assignment operator and a destructor; I assume those were left out for brevity, but if not they are terribly important.

As a1ex07 says in a comment, your code looks more like what you would put in operator= than in a copy constructor.

First of all, a copy constructor is initializing a brand new object. A check like if (this != &p) doesn't make much sense in a copy constructor; the point you are passing to the copy constructor is never going to be the object that you are initializing at that point (since it's a brand new object), so the check is always going to be true.

Also, doing things like if (label != NULL) is not going to work, because label is not yet initialized. This check might return true or false in random ways (depending if the uninitialized label is NULL by chance).

I would write it like this:

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

Maybe making label a std::string instead of a C-style char* would be a better idea, then you could write your copy constructor purely using an initializer list:

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

Yes -- I originally read it incorrectly.

However, I still suggest you use std::string for the label, as it will manage the memory for you and the copy constructor becomes much simpler (in fact, unnecessary for this case).