making a copy of data allocated by 'new'

[luckycusp]

class Triangle
{
  vertex *vx1,*vx2,*vx3;
  int index;
};

class vertex
{
  double x,y,z;

  vertex()
  {x=0,y=0,z=0;}

  vertex(double i,double j,double k)
  {x=i,y=j,z=k;}

};

int main()
{
  Triangle *start_triangle1=NULL;
  Triangle *start_triangle2=NULL;
  int i;

  start_triangle1 = new Triangle[100];

  for(i=0;i<100;i++)
  {
    /* assigning some random data to the vertices */
    (start_triangle1+i)->vx1 = new vertex(1.0*i,2.0*i,3.0*i);
    (start_triangle1+i)->vx2 = new vertex(4.0*i,5.0*i,6.0*i);
    (start_triangle1+i)->vx3 = new vertex(7.0*i,8.0*i,9.0*i);
    (start_triangle1+i)->index = i;
  }


  start_triangle2 = new Triangle[100];

}

Question: How can I make start_triangle2 get a copy of data being pointed by start_triangle1. (I dont want to simply make start_triangle2 = start_triangle1 assignment)

Another important thing is that, I would have to freshly allocate memory for the vertices for each triangle in the 2nd list (pointed by start_triangle2). But then also the old vertices values need to be copied. (sorry to make it so cryptic!) My objective is to be able work/modify with an exact similar dataset(copy of triangles data), without altering the original dataset (original triangles)

Should i use memcpy? If yes, please help me with usage of the command. It would be nice to see a best possible way to do it.

[S6k9GNh0]

1) Use initializer lists inside of your constructors.

2) In order to copy the data to start_triangle2, you'd have to allocate space for the pointer (such as start_triangle2 = new Triangle[100];) And then you could do something like memcpy or:

*start_triangle2 = *start_triangle1;
I've become a bit void to operations like this so correct me if this doesn't work.

[arrias]

Should i use memcpy? If yes, please help me with usage of the command. It would be nice to see a best possible way to do it.

With memcpy, you would probably do something like this:

memcpy((void *)start_triangle2, (void *)start_triangle1, sizeof(Triangle));

This copies the amount of bytes occupied by the Triangle class at the memory location pointed to by start_triangle1, to the location pointed to by start_triangle2 (Although make sure the destination memory is valid).

[Disch]

There are deeper problems here:

These classes are poorly encapsulated.

If there's dynamic allocation done in Triangle.. then it should be done in Triangle. main() and other outside code should not be responsible for allocating/cleaning up data in a class. It defeats the point of using a class.

That said -- you don't need (and probably shouldn't have) dynamic allocation for each vertex here. You're better off with a normal member var. Putting the index in Triangle doesn't make much sense either -- it forces Triangle to make an assumption of how it's going to be used. Triangle shouldn't worry about anything other than how to be a Triangle.

class vertex
{
  ...
  void Set(double xx,double yy,double zz)  // handy to set
  {
    x = xx;
    y = yy;
    z = zz;
  }
};

class Triangle
{
  // for member vars, this should be more or less all you need
  vertex vx[3];

  // maybe you could throw in some functions for calculations and whatnot as well.
};

//--------------------

int main()
{
  Triangle* start_triangle1 = new Triangle[100];

  for(i=0;i<100;i++)
  {
    // no need to dynamically allocate here, just use the Set() function:
    start_triangle1[i].vx[0].Set(1.0*i,2.0*i,3.0*i);
    start_triangle1[i].vx[1].Set(4.0*i,5.0*i,6.0*i);
    start_triangle1[i].vx[2].Set(7.0*i,8.0*i,9.0*i);
  }

  Triangle* start_triangle2 = new Triangle[100];

  // copy the triangle data with std::copy
  //  #include <algorithm>
  std::copy( start_triangle1, start_triangle1 + 100; start_triangle2 );
}

Note that std::copy will work for any object as long as you have a proper assignment operator. Your above Triangle class did not have a proper assignment operator due to you dynamiclaly allocating memory in it.

My Triangle above, however, will work fine with std::copy because there is no dynamic allocation. It will copy the verteces themselves rather than the pointers.

EDIT:

also, don't use memcpy. Not on objects. Ever.

EDIT2:

It's actually fine to memcpy POD objects. But I still advise against it. std::copy is the safer choice.

[somshekhar]

I think u can make use of copy constructor, if you are trying to make a deep copy.

[Disch]

The copy constructor will work for a single object copy.

Unfortunately it doesn't help with a 100 element array copy.

[luckycusp]

@Disch

I know it may be badly encapsulated but I am no expert in Object oriented programming. I just use classes the way it best helps in doing my task in hand. It makes no difference if the 'index' lement in the Triangle class is out of place! :)

Anyway, thanks for the advice but I plan to use memcpy. Could you please tell me why it could be unsafe if used with objects? What are POD objects?

@others

Thanks for your inputs.

[jsmith]

memcpy() does not run the constructor of the object, which means that if the object contains virtual methods
it will not be set up correctly. If you are using C++, do _not_ use memcpy.

POD objects -- objects that contain only POD types as data members. POD types -- plain 'ol data types like
bool, int, double, float.

[luckycusp]

What if I have 'vectors' within the class. can I use memcpy?

class Triangle
{
  vertex *vx1,*vx2,*vx3;
  vector<long int> ConnectedTriangleIDs; 
  int index;
};

[jsmith]

Absolutely not.

[luckycusp]

So what would be your suggestion, to make a copy of the above Triangle class?

[Disch]

I know it may be badly encapsulated but I am no expert in Object oriented programming

Which is why I brought it up. How will you learn if no one ever tells you ;P

I just use classes the way it best helps in doing my task in hand.

That's the thing. You weren't doing that. That's kind of my point.

Ideally a class should be self contained and impossible to misuse. Your Triangle class would have to be used a very specific way or else you will have all sorts of really bad problems (bad pointers, heap corruption, memory leaks, etc)

It may seem like no big deal now, but when your class is "open" like this.... when the entire program is responsible for taking care of Triangle (instead of Triangle just taking care of itself)... it's inevitable that you'll slip up, and have a program-killing bug. And since you give responsibility for Triangle to the entire program, it makes it extremely difficult to find and correct the source of the bug.

The whole point of OOP is to split up tasks into smaller parts.

It makes no difference if the 'index' lement in the Triangle class is out of place! :)

You may think that now... but you're wrong.

I doubt it really matters terribly if this is just some small test project you're throwing together, but I'd still argue it's bad practice.

Anyway, thanks for the advice but I plan to use memcpy

Why?

You really shouldn't. std::copy is better and just as easy to use.

So what would be your suggestion, to make a copy of the above Triangle class?

same thing as last time:

1) don't have vertex pointers -- instead just have a 3 element array of verteces
2) use std::copy

[jRaskell]

So what would be your suggestion, to make a copy of the above Triangle class?

As Disch already said, your classes as they stand in your original post are poorly designed, and the appropriate suggestion is, again as he already said, to redesign the classes themselves.

I just use classes the way it best helps in doing my task in hand.

In this case, you aren't even close to using classes the way it best helps in doing your task at hand. In fact, the way you're using classes is the very source of the problems with your code. If you really don't want to learn proper class usage and design, then you'll be better off just not using classes at all.

[luckycusp]

Hey Disch,

Thanks once again for the answer. The thing is, I am kindof screwed up coz of the bad implementation. I have written more than 3000 lines of code, using the above class definitions. (which i obviously did not post here)

Now its impossible for me to change the class declaration, and then rework each of the class-functions (around 20 functions!)

I know I am in a deep shit, as i just found out another BIG mistake I did while coding. For example, I needed to build a list of triangles for each triangle, which are adjacent/or touching. And check this function declaration,

vector< vector<Triangle*> > CreateAdjTriangleClusters(
	                                         Triangle *start_triangle, 
						 int nTriangles)

I am sure you understand where the Big mistake is. I create a list of addresses in vector< vector<Triangle*> >!! :'( It works fine actually, but its kindof impossible actually create a copy of the dataset, due to me using 'absolute' addresses.


I used index in triangle, so that I can assign an ID to each triangle. Many times I have to compare two triangles, or create a list.

Why?

You really shouldn't. std::copy is better and just as easy to use.

In general I prefer to work with raw data. Iterators confuse the hell out of me.


Anyway, I will come back once I fix the 'absolute addressing' mess first.

[jsmith]

In general I prefer to work with raw data. Iterators confuse the hell out of me.

I would suggest you stick to C then.

Reworking 20 member functions is not that bad.

[Disch]

Now its impossible for me to change the class declaration

Then you'll have to make your own "copy" function:

class Triangle
{
public:
  vertex *vx1,*vx2,*vx3;
  int index;

  void Copy(const Triangle& r)
  {
    vx1 = new vertex(*(r.vx1));
    vx2 = new vertex(*(r.vx2));
    vx3 = new vertex(*(r.vx3));
    index = r.index;
  }
};

But now this is risky because I'm using 'new' but not 'delete'. Which means you'll have to be sure to delete both the originals and the copies or else you'll have giant memory leaks.

Also, if you have a Triangle that was previously allocated, you'll need to delete its verteces before you call the above Copy function or else you have even more memory leaks.

This is the kind of thing that gets impossible to manage. Bugs and leaks are going to crawl out of the woodwork here. I really recommend you revamp your Traingle class. as jsmith said, 20 functions isn't so much (unless they're oversized -- which is another design problem). It might even be less work to do so.

Alternatively, you could write the above as an assignment operator so you can use std::copy. But I don't recommend it due to it being easy to misuse (poor encapsulation)

In general I prefer to work with raw data. Iterators confuse the hell out of me

std::copy is pretty much the same idea as memcpy:

std::copy( source_pointer, source_pointer + number_of_items_to_copy, dest_pointer );

At any rate... memcpy is simply not applicable to this situation at all. There's no way I can see to get it to work here.

[luckycusp]

I had to create my own Copy function. Tedious and highly risky( http://pastebin.com/m349d29e8 ), but works nevertheless!! Thank you all for your inputs.