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?

Is there a reason you can't re-define `visibleShape` as: `std::vector visibleShape`, and delete your copy ctor completely? — Jerry Coffin, Apr 20 '10 at 18:56
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. — CB Bailey, Apr 20 '10 at 18:59
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. — CB Bailey, Apr 20 '10 at 19:08
@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! — matt, Apr 20 '10 at 19:12
@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`). — Mike Dinsdale, Apr 20 '10 at 19:20
@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. — Jerry Coffin, Apr 20 '10 at 19:38

score 2 · Accepted Answer · answered Apr 20 '10 at 18:56

2

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

answered Apr 20 '10 at 18:56

Mike Dinsdale

1,491
9
8

score 1 · Answer 2 · answered Apr 20 '10 at 18:58

1

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

Hyp& operator = (const Hyp& rhs);

answered Apr 20 '10 at 18:58

Andreas Brinck

51,293
14
84
114

How does the implementation of this function look like? It is obviously necessary to make use of the copy constructor, isn't it? – arjacsoh Dec 25 '11 at 16:07

vector::erase with pointer member

2 Answers2

Linked