c++ Copy constructor and pointer

1
votes

I'm having a class Place with such methods :

class  Place{

    protected:
    Keypoint* _kp;

    Place() {
        Keypoint* kp = new Keypoint();
        _kp = kp;
    };
    Place(const Place& cSource){
        delete _kp;
        _kp = cSource.makePointer();

        }
    virtual ~Place(){
        delete _kp;
    }

    virtual Keypoint* makePointer() const {
        return _kp->makePointer();
    }


};

Thus a constructor, a destructor and a copy method.

In my main I do something as simple as AASS::graphmatch::Place ppp; AASS::graphmatch::Place p2(ppp); to try the copy constructor and I have a huge segfault with valgrind telling me that I use Conditional jump or move depends on uninitialised value(s) after the delete in the copy. The method makePointer in Keypoint does a new and return a pointer Keypoint*. 

virtual Keypoint* makePointer() const {
    Keypoint* d = new Keypoint();
    return d;
}

It is my understanding that every new should be freed using delete so that's why I use delete before calling makePointer. So that's why I start by deleting _kp to release the old memory and then I assign it the newly created pointer.

I can't get my head around why I have uninitialized value ?

3
c++pointerscopy-constructordelete-operator
when you delete _kp; in your copy constructor, the _kp has not been initialised for the new object yet, so you're trying to delete <garbage-ptr> - BeyelerStudios
Is this really a copy constructor? It seems as Keypoint is default created every time (unless you use some unseen global variable). This seems to violate the x == y principle... - patrik
I'm not sure what the x==y principle is in this case :S, but what I do is that I have some class that inherit from Keypoint and instantiate pointer to their own class using makePointer(). So depending on cSource type I obtain a pointer of cSource class which is also a Keypoint. - Malcolm
@Malc The comment was a bit sloppy, but my point is that if you have a copy constructor you would probably want the copy copy constructed object to be somewhat equal to the old object. Assume Place p {}; Place p2{p}; bool eq = p==p2;, then eq should be true. You can of course define the equality operator as you want but in this case every "copy" you get would look the same as an default constructed object, which I in my view is not much of a copy. In Particular you want f(p) and f(p2) to give the same answer and that would be hard if you have done changes to p before copy construction. - patrik
@patrik I see what you mean :)! Here it's fine because every keypoint, once created, can't be changed. They contain stuff like type which is defined by a string which is the same for same keypoints or color which is the same for same keypoints. So, in my application, once created, a keypoint can never be changed. But thanks for pointing it out because, if I change it, I wouldn't have realized it and I probably would have made the mistake :). - Malcolm

3 Answers

3
votes

In your copy constructor, you are calling delete _kp;. But _kp has not been initialised at that point.

That will not end well.

Remove the line delete _kp;. Better still, consider using base member initialisation:

Place(const Place& cSource) : _kp(cSource.makePointer())
{
}

Even better than that, consider using a managed pointer class like std::unique_ptr for your Keypoint member.

3
votes

You wrote your copy constructor as if it were an assignment operator:

Place(const Place& cSource){
    delete _kp;
    _kp = cSource.makePointer();

    }

You may need an assignment operator, and you may also need a copy constructor. But they aren't the same thing.

On entry to the copy constructor, _kp holds garbage, so that delete is a bug.

1
votes

You delete an uninitialzed pointer in your copy constructor:

Place(const Place& cSource){
    // delete _kp; // _kp is not initialized 
    _kp = cSource.makePointer(); // now _kp gets initialized
}