Inserting in a list changes values c++

Question

I want to create multiple "voxel"-objects and put them in a list. But if I do so two of the values, that are stored by voxel, change. Here is the class definition:

class Voxelization{

private:
int sf_counter;
bool subdiv;
int seg_fac;
int* labels;
Pixel<int>* centers;
int n_seeds;
float* pixels;
float* depth;
int width;
int height;
int iterations;
float r1;
float r2;
int env_size;
bool move_centers;
typename pcl::PointCloud<PointT>::ConstPtr& cloud;
typename NormalCloudT::ConstPtr normal_cloud;
}

And this is the copy constructor:

Voxelization::Voxelization(const Voxelization& voxel):labels(voxel.labels), centers(voxel.centers),n_seeds(voxel.n_seeds),pixels(voxel.pixels),depth(voxel.depth),width(voxel.width),height(voxel.height),iterations(voxel.iterations),r1(voxel.r1),r2(voxel.r2),env_size(voxel.env_size),move_centers(voxel.move_centers),cloud(voxel.cloud){}

And with this code I'm inserting them:

  int seg_fac=100;
  int sf_counter=0;
  std::list<Voxelization> voxels
  for(sf_counter; sf_counter<seg_fac;sf_counter++){
      Voxelization voxel(input_cloud_ptr,seg_fac,sf_counter);
      voxels.push_back(voxel);
  }

If I look at the variables, after I create the single voxel-objects, the values for seg_fac and sf_counter are right. But if I then insert them, and look them up in the list they change to something like 8744512 or -236461096. So my question is, where does this come from?

Any help is appreciated, and thanks in advance.

The constructor you show is not very interesting - the copy constructor would be. Also, the class definition would help. My educated guess: your class has a lot of members that are pointers, which you allocate in your constructor, and likely deallocate in the destructor. You don't have an explicit copy constructor, so the compiler-generated one just copies pointers. So you end up with two instances holding the same pointer, then one of them is destroyed and deallocates memory behind that pointer, and the other is left with a dangling pointer. — Igor Tandetnik, Jul 20 '16 at 13:35
`int* empty_labels = new int[width*height];` -- Why aren't you using `std::vector empty_labels(width*height, 0)`? Same thing with the rest of the code that does `new[]`. You could use `std::vector` and not need to do this. This may not sound important, but could be part and parcel of why you're having issues with `voxels.push_back(voxel)`. — PaulMcKenzie, Jul 20 '16 at 13:39
See also: [Rule of Three](https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)) — Igor Tandetnik, Jul 20 '16 at 13:40
@PaulMcKenzie I'd like to do it this way, but i can't because I have to work with another function, that only supports these inputs. And this function is not written by me. — Sven, Jul 20 '16 at 13:42
Almost as I thought: you do have an explicit copy constructor after all, but all it does is copy pointers, leaving two instances both thinking they own the same memory. — Igor Tandetnik, Jul 20 '16 at 13:42
Igor has it. When you insert into the list, you "copy construct" the object, then delete the original. Since your class has many raw pointers, they will not copy correctly. Precisely why it doesn't copy two specific items we can't know without seeing their definitions, but we already know most of the class will fail at the first copy. — Kenny Ostrom, Jul 20 '16 at 13:43
@sven -- You do know that passing the address of the first element in a vector is the same as passing a pointer to a buffer? So the excuse that the other functions have to use this format doesn't hold water. `set_depth(depth.data())` If `data` is a vector. — PaulMcKenzie, Jul 20 '16 at 13:44
Thanks to all of you, I'm new to C++ so I didn't know that the pointers are a problem if I just copy them. I will read up on "Rule of Three" and change my code! — Sven, Jul 20 '16 at 13:45
@PaulMcKenzi No I didn't know, but shouldn't be an excuse. Thanks! — Sven, Jul 20 '16 at 13:46
You wouldn't need the `Rule of Three` if you used types that knew how to properly copy themselves. Changing the raw pointers to safely-copyable types removes the need to write a copy constructor, assignment operator, and destructor. — PaulMcKenzie, Jul 20 '16 at 13:47

score 0 · Accepted Answer · answered Jul 21 '16 at 13:20

After fiddling around with my constructers, destructors and the operator= I found out that I simply forgot to add

int sf_counter;
bool subdiv;
int seg_fac;

to my copy-constructor. So the working copy-constructor looks like:

Voxelization::Voxelization(const Voxelization& voxel):sf_counter(voxel.sf_counter),subdiv(voxel.subdiv),seg_fac(voxel.seg_fac),labels(new int[voxel.width*voxel.height]), centers(voxel.centers),n_seeds(voxel.n_seeds),pixels(new float[voxel.width*voxel.height*3]),depth(new float[voxel.width*voxel.height]),width(voxel.width),height(voxel.height),iterations(voxel.iterations),r1(voxel.r1),r2(voxel.r2),env_size(voxel.env_size),move_centers(voxel.move_centers),cloud(voxel.cloud),normal_cloud(voxel.normal_cloud){
std::copy(voxel.labels, voxel.labels + voxel.width*voxel.height, labels);
std::copy(voxel.pixels, voxel.pixels + voxel.width*voxel.height*3, pixels);
std::copy(voxel.depth, voxel.depth + voxel.width*voxel.height, depth);
std::copy(voxel.centers, voxel.centers + voxel.width*voxel.height, centers);
}

Inserting in a list changes values c++

1 Answers1