vector::erase with pointer member

Question

I am manipulating vectors of objects defined as follow:

class Hyp{
public:
int x;
int y;
double wFactor;
double hFactor;
char shapeNum;
double* visibleShape; 
int xmin, xmax, ymin, ymax; 

Hyp(int xx, int yy, double ww, double hh, char s): x(xx), y(yy), wFactor(ww), hFactor(hh), shapeNum(s) {visibleShape=0;shapeNum=-1;};

//Copy constructor necessary for support of vector::push_back() with visibleShape
Hyp(const Hyp &other)
{
    x = other.x;
    y = other.y;
    wFactor = other.wFactor;
    hFactor = other.hFactor;
    shapeNum = other.shapeNum;
    xmin = other.xmin;
    xmax = other.xmax;
    ymin = other.ymin;
    ymax = other.ymax;
    int visShapeSize = (xmax-xmin+1)*(ymax-ymin+1);
    visibleShape = new double[visShapeSize];
    for (int ind=0; ind<visShapeSize; ind++)
    {
        visibleShape[ind] = other.visibleShape[ind];
    }
};

~Hyp(){delete[] visibleShape;};

};

When I create a Hyp object, allocate/write memory to visibleShape and add the object to a vector with vector::push_back, everything works as expected: the data pointed by visibleShape is copied using the copy-constructor.

But when I use vector::erase to remove a Hyp from the vector, the other elements are moved correctly EXCEPT the pointer members visibleShape that are now pointing to wrong addresses! How to avoid this problem? Am I missing something?

Antimatter

Is there a reason you can't re-define visibleShape as: std::vector<double> visibleShape, and delete your copy ctor completely? – Engler

You don't seem to initialize xmin, xmax, etc. or allocate visibleShape in the default constructor but you use calculation on xmin, xmax, etc. to determine that you can read from the other object's visibleShape` in the copy constructor. – Roam

Also, all your data members are public so you have no control over whether it's valid to delete[] visibleShape in your destructor; it could have been assigned to anything. You should consider using a vector or at least make it private so that you have a chance to enforce your class invariants. – Roam

@Jerry Coffin: I'm using visibleShape to store portions of image of size determined by the other members of Hyp. Isn't it more efficient to use a simple pointer instead of a vector? I am new to C++ so I am open to suggestions! – Antimatter

@matt: If there's a difference in efficiency it's going to be tiny - the vector is just doing automatically the stuff you otherwise need to do manually... remember you can use the vector constructor that takes a size parameter, or the resize method, if you know the size in advance (or reserve if you want to use push_back). – Colby

Thank you for all these very useful comments! – Antimatter

@matt:no, doing the memory management on your own won't normally improve speed (enough to care about). It might even hurt it a little. In the case of a primitive type like double, there probably won't be any difference, but for types that have ctors, vector will usually be faster. – Engler

Answer 1

I think you're missing an overloaded assignment operator for Hyp.

Colby

Answer 2

I think you might be missing the assignment operator = in the Hyp class.

Hyp& operator = (const Hyp& rhs);

Razid

How does the implementation of this function look like? It is obviously necessary to make use of the copy constructor, isn't it? – Anatolia