memcpy(): why does this work?

[testalucida]

Hi all,
would sb be so kind, have a look at following code and answer my questions (see comments)

#include <iostream>
#include <string>
using namespace std;

void main() {
  //allocate an array of 3 strings:
  int ARRAYSIZE = 3;
  string *pArr = NULL;
  int sz = sizeof( string );  // <-- sz == 4
  pArr = (string*)realloc( pArr, ARRAYSIZE * sz );
  
  //define a pointer and make it point to the begin of the array:
  string *pTmp = pArr;

  //string to copy:
  string ts = "teststring";
  sz = sizeof( ts );  // <-- sz == 4; WHY?? shouldn't this be at least 10???

  //copy ts 3 times into the array:
  for( int i = 0; i < ARRAYSIZE; i++ ) {
    pTmp = &pArr[i];
    memcpy( pTmp, &ts, sz ); // <-- should copy 4 bytes,
			 //but ts has been completely copied - WHY?
  }

  //print the array:
  for( int i = 0; i < ARRAYSIZE; i++ ) {
    fprintf( stderr, "p[%d] = %s\n", i, pArr[i].c_str() );
  }
}

Seems as if I had allocated 12 bytes and then copied 30 bytes into it and all is working fine. Who can lighten up my darkness?

Thanks in advance!

[kbw]

Where do I begin? Ok, here we go.

Why do you think it works? You done some pretty awful things there. Just because the program didn't crash doesn't mean it works.

string is an object. Without looking at the implementation, you won't know how it works. And so you can't make assumption about its size or much else.

pArr = (string*)realloc( pArr, ARRAYSIZE * sz );

This is dangerous. You've overridden the type system and insisted that this memory block you got back from realloc isn't raw memory, but a constructed string object (or array of string objects).

You then copy the implementation of a properly constructed string into this array:

memcpy( pTmp, &ts, sz );

So now you have ARRAYSIZE strings that are bitwise copies of ts.

You then use these copies by calling a const method (that promises not to change the object).

Then you stop. ts gets released and you leak the allocated memory. But what you're left with are a number of dangling references.

[Disch]

This code is flawed in many many many ways.

General rules:

1) don't use malloc/realloc/memcpy with complex types like string
2) don't use sizeof() to try and get the length of the string. Use string's builtin length function

  int ARRAYSIZE = 3;
  string *pArr = NULL;
  int sz = sizeof( string );
  pArr = (string*)realloc( pArr, ARRAYSIZE * sz );  // <- FATAL
   // this allocates SPACE for the strings, but does not actually CREATE any strings
   //  ie:  their construtor is never called.

   string *pTmp = pArr;  // <- this is just kind of pointless

  string ts = "teststring";
  sz = sizeof( ts );  // <-- sizeof(string) gives you the size of the string class, which does not change with the
  //  length of the string (the size of an object NEVER changes).  You want ts.size() or ts.length()

  for( int i = 0; i < ARRAYSIZE; i++ ) {
    pTmp = &pArr[i];
    memcpy( pTmp, &ts, sz ); // <- FATAL, again 'pTmp' is pointing to valid memory, but it is NOT pointing to a
       // string.  This is very very bad.
  }

  for( int i = 0; i < ARRAYSIZE; i++ ) {
    fprintf( stderr, "p[%d] = %s\n", i, pArr[i].c_str() );  // <- FATAL, calling c_str on something that is not a string
       // is bad.
  }
}

You are likely corrupting the heap by doing all this. And you'll possibly cause program crashes and other oddities.

Never try to use direct memory manipulation with complex objects. It's a horrible idea.

This task is much, much simpler if you just use string how it is intended:

int main() {  //<- main must return an int
  int ARRAYSIZE = 3;
  string *pArr = NULL;
  // int sz = sizeof( string );  // <-- no need for this nonsense
  pArr = new string[3];  // make 3 strings.  Note this actually works, and calls ctors... whereas realloc will not
  
  // string *pTmp = pArr; // <- no need for this

  string ts = "teststring";
  // sz = sizeof( ts );  // <-- no need for this

  for( int i = 0; i < ARRAYSIZE; i++ ) {
    // pTmp = &pArr[i];
    //  memcpy( pTmp, &ts, sz ); // <- no need for any of this crap
    pArr[i] = ts;  // <- just assign it normally
  }

  for( int i = 0; i < ARRAYSIZE; i++ ) {
    fprintf( stderr, "p[%d] = %s\n", i, pArr[i].c_str() );  // <- now this will work fine
  }

  delete[] pArr;  // <- don't forget to delete[] things that you new[]'d.
     //  Just like you must free() things you malloc()'d
}

[Gaminic]

My IDE closed itself when I opened this page...

[roberts]

1) don't use malloc/realloc/memcpy with complex types like string

I would go into further detail and say don't use malloc on class data types. Structs would be fine, provided they don't need a constructor and destructor. In short, if it has a constructor, use new, not malloc.

@OP, strings are not char arrays. They aren't the same at all.

[testalucida]

thanks to you all.
and my condolences to gaminic - didn't intend to damage his IDE ;)

[moorecm]

I would avoid malloc (and family) like the plague in C++. Even for structs it is most often less clear, more error prone, harder to maintain, and no more efficient.

[roberts]

If I foresee having to realloc, what is the C++ method for that?

[Disch]

If I foresee having to realloc, what is the C++ method for that?

Using a vector

vector<string> foo(10);  // array of 10 strings

foo[4] = "whatever";

foo.resize( 20 );  // make it 20 strings 

EDIT

roberts wrote:

I would go into further detail and say don't use malloc on class data types

This is basically what I meant by "complex" data types. Anything other than built-ins, really.

roberts wrote:

In short, if it has a constructor, use new, not malloc.

Rather than mix and match and have to remember which ones you can use, and where you used each one... I'd say just dont' use malloc in C++ ever. It really doesn't have any place.

moorecm wrote:

I would avoid malloc (and family) like the plague in C++.

+1 @ this.

[roberts]

And for a 2D array?

[Disch]

I avoid multidimentional arrays like the plague for various reasons.

But you can have vectors of vectors:

vector< vector<int > > foo( 10, vector<int>( 20 ) ); // int[10][20] 

Or you can simulate it in a 1D array and wrap the functionality in a class.

See end of this article for an example: http://www.cplusplus.com/articles/G8hv0pDG/

[vlad from moscow]

Disch .
you can use a more simple construcrion to initialize a vector of vector. Instead of your code

vector< vector<int > > foo( 10, vector<int>( 20 ) ); // int[10][20]

you can write

vector< vector<int > > foo( 10, 20 ); // int[10][20]


Do ypu agree that this is much simpler?

[Disch]

Yes, apparently you are right. I didn't think that would work but it does.

[moorecm]

Same here. That's a very interesting initialisation--it makes sense but it's easy to overlook!

[roberts]

I avoid multidimentional arrays like the plague for various reasons.

I don't. If a programmer defines his 2D arrays as being very discreet and/or passes along the dimensions, there is no problem.

I don't see there being a problem with 2D arrays.

But you can have vectors of vectors:

vector< vector<int > > foo( 10, vector<int>( 20 ) ); // int[10][20]  

That would be death for a game requiring high performance, which is what I'm working toward. For what I'm doing, I need only 3x3 and/or 4x4 matrices. Anything outside of this would get their own functions, but it would also be the case that any matrix outside of 3x3 or 4x4 would not be something in the inner loop.

[Disch]

roberts wrote:

I don't. If a programmer defines his 2D arrays as being very discreet and/or passes along the dimensions, there is no problem.

I suppose it's a preference thing. But I find that passing multidimentional (or even 1-D) arrays between functions is wretched.

About the only thing MD arrays are good for is for lookup tables, and you don't have to pass those around. Any runtime data that needs to be put in a MD array would be better represented in a class that wraps around the concept.

For example, instead of having a 3x3 array to represent a matrix, you're better off having a Matrix class. Or instead of a 100x100 array to represent a game map, you're better off with a Map class. How those classes represent the data internally doesn't really matter, though I still prefer to have 1D arrays and mimic 2D functionality with it.

That would be death for a game requiring high performance, which is what I'm working toward

Then make a 1D array and mimic a 2D array with it:

// instead of:
vector< vector<int> > foo(10,20);
foo[1][2] = x;

// do this:
vector<int> foo(10 * 20);
foo[ (1 * 20) + 2 ] = x;

But again, it's better if that's wrapped in a class to make it less error prone.

See the Array2D class here for an example:
http://cplusplus.com/forum/articles/17108/#msg85595