copy-constructor issue

[carlossg]

Hi There,
Can anyone explainme why this piece of code doesn´t work?
I'm running VC 6.0 / W2K.



#include
#include
#include
#include

using std::string;
using std::cout;
using std::vector;
using std::ostream;
using std::endl;

class Dog {
string nm;
public:
Dog ( void ) : nm("NO NAME") {}
Dog(const string& name) : nm(name) {
cout << "Creating Dog: " << *this << endl;
}
// Synthesized copy-constructor & operator=
// are correct.
// Create a Dog from a Dog pointer:
Dog(const Dog* dp, const string& msg)
: nm(dp->nm + msg) {
cout << "Copied dog " << *this << " from "
<< *dp << endl;
}
~Dog() {
cout << "Deleting Dog: " << *this << endl;
}
void rename(const string& newName) {
nm = newName;
cout << "Dog renamed to: " << *this << endl;
}
friend ostream&
operator<<(ostream& os, const Dog& d) {
return os << "[" << d.nm << "]";
}
};

class DogHouse {
Dog* p; //Dog array
string houseName;
public:
DogHouse (Dog * lpDog, const size_t size) : p(new Dog[size]) {
memcpy(p, lpDog, size);
}
DogHouse(Dog* dog, const string& house)
: p(dog), houseName(house) {}
DogHouse(const DogHouse& dh)
: p(new Dog(dh.p, " copy-constructed")),
houseName(dh.houseName
+ " copy-constructed") {}
DogHouse& operator=(const DogHouse& dh) {
// Check for self-assignment:
if(&dh != this) {
p = new Dog(dh.p, " assigned");
houseName = dh.houseName + " assigned";
}
return *this;
}

void renameHouse(const string& newName) {
houseName = newName;
}
Dog* getDog() const { return p; }
~DogHouse() { delete p; }
friend ostream&
operator<<(ostream& os, const DogHouse& dh) {
return os << "[" << dh.houseName
<< "] contains " << *dh.p;
}
};

int main() {

Dog arrDog[2][10] = {
{ Dog("01"),Dog("02"),Dog("03"),Dog("04"),Dog("05") },
{ Dog("11"),Dog("12"),Dog("13"),Dog("14"),Dog("15") }
};
vector HouseContainer;
/*HouseContainer.push_back(DogHouse(new Dog(arrDog[0]," From static array"), "Static array"));*/
HouseContainer.push_back(DogHouse(arrDog[0], sizeof(Dog)));
/*HouseContainer.push_back(DogHouse(new Dog("Kido"), "KidoHouse"));*/

std::copy(HouseContainer.begin(),
HouseContainer.end(),
std::ostream_iterator(cout,"\n"));

return 1;
}




Assert Memory error raised when trying to do bookeeping.
I guess the problem is how static array of 'Dogs' is copied (using memcpy) to the new object 'DogHouse'.

Can anyone tell me how can I make this idea work without changing the interface, I mean I would like to keep using Dog pointer to store the array of Dog instead of a vector or other containers.

Thanks in advanced.
Carlos.

[Graham]

I mean I would like to keep using Dog pointer to store the array of Dog instead of a vector or other containers.

Why?

Bare pointers are intrinsically dangerous and prone to leaks. I can see no reason to prefer using new'd memory and a bare pointer over an STL container.

[Paul McKenzie]

My first question is why do you state that you don't want to use vector? Unless there is a compelling reason to use pointers, you should use vector<> instead of pointers. At least have the class use all vectors internally.

For example, why is it necessary to keep a Dog* around as the container to hold a dog? Just copy the Dog* data to a Dog vector, and that's all you have to do. Then the internals of your class doesn't need to do all of this pointer manipulation.

I will illustrate the first point with some more observations:

a) Your call to delete is incorrect. It should be delete[], not delete. Allocate with new[], deallocate with delete[].

b) The constructor that takes the size argument -- how do you know that the size being passed is valid? There is no check for a negative value, a 0, etc.

c) Your usage of memcpy() is incorrect, it is incorrect to use it for the object that you are copying. The Dog class is not a POD type (Plain Old Data), i.e. an int, double, char, float, or a structure that just contains these types or array of these types. Dog contains a std::string. Just this fact alone shows that memcpy() shouldn't be used on this class. The memcpy() function should be used on POD types only. You need to construct the copies correctly, and memcpy() does not call the copy constructors for the objects.

d) You have various places where you dont delete[] the old dog pointer. For example, your DogHouse::operator=() doesn't delete[] the old p, instead it just allocates for a new p -- of course this is a memory leak.

You could have prevented all of these problems if you used vector. Vectors know to copy correctly, there is no need to be manipulating dynamic memory, etc. Just the mere fact that you are having problems and the number of errors that I found is enough to just scrap what you're doing and just use the vector<> class. It's there, it's standard, it works, it's been coded by some of the best coders in the C++ world, so why not use it?
---------------------------------------------
class Dog {
string nm;
public:
Dog ( void ) : nm("NO NAME") {}

Dog(const string& name) : nm(name)
{ }

Dog(const Dog& dp, const string& msg)
: nm(dp.nm + msg)
{ }

// Define a true copy constructor here
Dog(const Dog& dp)
: nm("NO NAME") { }

~Dog()
{ }

void rename(const string& newName)
{
nm = newName;
}

friend ostream& operator<<(ostream& os, const Dog& d)
{
return os << "[" << d.nm << "]";
}
};

typedef std::vector<Dog> DogContainer;

class DogHouse
{
DogContainer p; //Dog array
string houseName;

public:
DogHouse (Dog * lpDog, const size_t size)
{
if ( size >= 0 )
{
p.resize(size);
std::copy(lpDog, lpDog + size, p.begin());
}
}

DogHouse(const Dog& dog, const string& house)
: p(1, dog), houseName(house) { }

// No need for copy constructor, op=, or destructor

void renameHouse(const string& newName)
{
houseName = newName;
}

// Why are you returning a pointer to your private data? This makes your
// private data now accessible to the public, making your class susceptible to all sorts of problems.
/*
DogContainer* getDog() const { return &p[0]; } // I *wouldn't* do this!
*/
DogContainer getDog() const { return p; } // Makes more sense

// What were you trying to output here?
friend ostream&operator<<(ostream& os, const DogHouse& dh)
{
return os << "[" << dh.houseName << "] contains ";
// ??
}
};

I didn't compile the code, test it, or implemented some functions that you may want, but it shows how much can be eliminated from your current code with the proper usage of the standard library. Note that as soon as the constructor was called that called for a pointer and a size, I merely copied over the data to a vector, and everything is simple from there. For this to work, your main() function has to call the constructor correctly -- a pointer to dogs, and how many dogs. Currently, you are passing the pointer to the dog array and the number of bytes that a Dog object contains, not the number of dogs in the array.

int NumberOfDogs = sizeof(arrDog) / sizeof(Dog);
HouseContainer.push_back(DogHouse(arrDog[0], NumberOfDogs));


The interface of the library can stay the same, but the way the library works internally is what is important.

Regards,

Paul McKenzie

[carlossg]

Hi There,
Thanks for the replies.

First of all, apologizes for the previous chunck of code, it was all messed up and did not make much of sense.

The reason because a wanted to keep using 'pointers' instead of a 'vector container' was because of the initialization array.

arrDog [2][5] = {
{ Dog("00"), Dog("01"), Dog("02"), Dog("03"), Dog("04"), }
{ Dog("10"), Dog("11"), Dog("12"), Dog("13"), Dog("14") }
{......}
}


In this case initialization array is very small and could easily be changed using vector declaration.

vector <Dog> arrDog[2];
arrDog[0].push_back(Dog("00"));
arrDog[0].push_back(Dog("01"));
etc...

Any way to avoid this anoying declaration?


In my real case, arrDog is about [100][100] and the class used isn´t as simple as Dog.

That is the only reason, just avoid rewriting all the stuff again.
I know this is a *very* poor reason, please don´t flame at me :)

Althought I will use vectors I have found that using std::copy instead of memcpy things seems to work a bit better.

Does anybody know the gory details of std::copy?
Does std::copy calls copy-constructors members?
Using memcpy instead of std::copy I get rubish when printing stuff saved in vectors but I don´t get memory error, any reason?



#include <string>
#include <iostream>
#include <vector>
#include <algorithm>

using std::string;
using std::cout;
using std::vector;
using std::ostream;
using std::endl;

class Dog {
string nm;
public:
Dog ( void ) : nm("NO NAME") {}
Dog(const string& name) : nm(name) {
cout << "Creating Dog: " << *this << endl;
}

Dog(const Dog& d) : nm("copy " + d.nm) {
cout << "Copy-constructed Dog " << nm << endl;
}

~Dog() {
cout << "Deleting Dog: " << *this << endl;
}

friend ostream&
operator<<(ostream& os, const Dog& d) {
return os << "[" << d.nm << "]";
}
};

class DogHouse {
Dog * m_p; //Dog Array
int m_n; //Number of dogs
string m_houseName;
public:
DogHouse (const Dog * p, const int n , const string& hn)
: m_p(new Dog[n * sizeof(Dog)]) ,m_n (n), m_houseName(hn) {
std::copy(p, p + n, m_p);
//memcpy(m_p, p , n * sizeof(Dog)); Uncomment this
}

DogHouse(const DogHouse& dh)
: m_p(new Dog[dh.m_n * sizeof(Dog)]), m_n(dh.m_n),
m_houseName(dh.m_houseName + " copy-constructed") {
std::copy(dh.m_p,dh.m_p+dh.m_n,m_p);
//memcpy(m_p, dh.m_p, dh.m_n * sizeof(Dog)); Uncomment this
}

~DogHouse() { delete [] m_p; }
friend ostream&
operator<<(ostream& os, const DogHouse& dh) {
os << "[" << dh.m_houseName << "] contains " << endl;
std::copy(dh.m_p,dh.m_p + dh.m_n,
std::ostream_iterator<Dog>(os,"\n"));
return os;
}
};

int main() {

Dog arrDog[4][5] = {
{ Dog("01"),Dog("02"),Dog("03"),Dog("04"),Dog("05") },
{ Dog("11"),Dog("12"),Dog("13"),Dog("14"),Dog("15") },
{ Dog("21"),Dog("22"),Dog("23"),Dog("24"),Dog("25") },
{ Dog("31"),Dog("32"),Dog("33"),Dog("34"),Dog("35") }
};

vector <DogHouse> HouseContainer;
HouseContainer.push_back(DogHouse(arrDog[0], 5 ," Dog Row 0 "));
HouseContainer.push_back(DogHouse(arrDog[1], 5 ," Dog Row 1 "));
HouseContainer.push_back(DogHouse(arrDog[3], 5 ," Dog Row 3 "));


std::copy(HouseContainer.begin(),
HouseContainer.end(),
std::ostream_iterator<DogHouse>(cout,"\n"));
return 1;
}



Regards
Carlos.

[Graham]

As you say, convenience of initialisation is not a good reason for using pointers rather than vectors (why should that have so much greater weighting than the rest of the code?)

Your DogHouse Constructor could still take an array and fill a vector:

class DogHouse // Non-essential stuff snipped out....
{
std::vector<Dog> dogs_;
std::string name_;

public:
DogHouse(const Dog* first, const Dog* last, const std::string& name)
: dogs_(first, last), name_(name)
{}
};

int main()
{
Dog doglist[] = {Dog("Dog1"), Dog("Dog2"), Dog("Dog3")};

DogHouse dh(doglist, doglist+3, "House1");

// ...
}

This uses the vector ctor that takes a pair of iterators to specify a range from which to initially construct the vector.

It shouldn't be too hard to generalise this, and to supply a function that can read your intialiser list and fill in the whole vector<DogHouse>. Since this ought to be a one-off, it's hardly an onerous task. Also you wouldn't be wasting space if you have DogHouses with differing numbers of dogs.

[Paul McKenzie]

Originally posted by carlossg
Hi There,
Thanks for the replies.
<snip>
Althought I will use vectors I have found that using std::copy instead of memcpy things seems to work a bit better.

Does anybody know the gory details of std::copy?
Does std::copy calls copy-constructors members?
Using memcpy instead of std::copy I get rubish when printing stuff saved in vectors but I don´t get memory error, any reason?
<snip>
Read my explanation of why memcpy() does not work. You can't copy a non-POD (Plain Old Data) type using memcpy(). Here is a much simpler example of what you were trying to do:

#include <string>
#include <stdio.h>
int main()
{
std::string s1 = "Tom";
std::string s2;

// This will not work the way you expect
memcpy(&s2, &s1, sizeof(std::string));

// This is correct, but you may have messed up s2 with the memcpy(), making this function useless!!
s2 = s1;
}

The simple reason is that the std::string is a non-POD type, and all memcpy() does is do a bitwise copy. The problem with this is that you don't know how std::string is implemented. A bitwise copy may not be suitable or even desirable if you want to copy a string to another string. What if the string is reference counted? Copying a string using memcpy() would be a disaster. What if the object needs to do houskeeping when a copy is done? Again, memcpy() would be a disaster.

Therefore the correct way to copy non-POD types is to use operator =. The std::copy function invokes the proper semantics to copy objects using the object's operator =. It may also be specialized to do a memcpy() if std::copy recognizes that the type to copy is a POD type.

Also, Graham is basically saying the same thing as I'm saying. There is no reason to be holding onto pointers *within* your class. If your public interface calls for a pointer, use the pointer to fill the vector -- you don't need to be carrying the pointer around in your class. As a user of your DogHouse class, why should I care how you store my data? If you use a vector, list, map, set, it doesn't matter to the user. Therefore why not use safer constructs such as vector?

Regards,

Paul McKenzie