• 0

[C++] Copy Constructors


Question

I'm writing a copy constructor, and I've never done one before so hopefully am aboot there.

Anyway I have in my class a buffer that is created during construction.

Classname::Classname(UINT timer, int nRecieveBufferSize)
{
	m_iTimeoutInMiliseconds = timer;
	//Buffer must not be less than 20k, but can be larger
	if (nRecieveBufferSize < 20000)
		nRecieveBufferSize = 20000;
	nBufferSize = nRecieveBufferSize;
	m_pbRecieveBuffer = new BYTE[nRecieveBufferSize];
	//Zero the buffer.
	ZeroMemory(m_pbRecieveBuffer, nRecieveBufferSize);
}

For the copy constructor I have

Classname::Classname(Classname& pc)
{
	m_pbRecieveBuffer = new BYTE[pc.GetBufferSize()];
	ZeroMemory(m_pbRecieveBuffer, pc.GetBufferSize());
	nBufferSize = pc.GetBufferSize();
	m_pbRecieveBuffer = pc.m_pbRecieveBuffer;
}

also

Classname& Classname::operator=(Classname &pc)
{
	if(this == &pc)
	{
		return *this;
	}
	else 
	{
		m_pbRecieveBuffer = new BYTE[pc.GetBufferSize()];
		ZeroMemory(m_pbRecieveBuffer, pc.GetBufferSize());
		nBufferSize = pc.GetBufferSize();
		m_pbRecieveBuffer = pc.m_pbRecieveBuffer;
		return *this;
	}
}

Right now...is this right? :p

Link to comment
Share on other sites

6 answers to this question

Recommended Posts

  • 0

You have a few problems:

1. In both the copy ctor and assignment operator you have a memory leak, it should be:

delete[] m_pbRecieveBuffer;
m_pbRecieveBuffer = new BYTE[pc.GetBufferSize()];

2. In your copy ctor you can simplify it a bit:

Classname& Classname::operator=(const Classname &pc)
{
	if(this != &pc)
	{
		delete[] m_pbRecieveBuffer; // Free current memory
		nBufferSize = pc.GetBufferSize(); // Set the buffer size
		m_pbRecieveBuffer = new BYTE[nBufferSize];
		memcpy(m_pbRecieveBuffer, pc.m_pbRecieveBuffer, nBufferSize); // copy from pc
	}
	return *this;
}

So your copy ctor should be:

Classname::Classname(const Classname& pc)
{
	nBufferSize = pc.GetBufferSize();
	m_pbRecieveBuffer = new BYTE[nBufferSize];
	memcpy(m_pbRecieveBuffer, pc.m_pbRecieveBuffer, nBufferSize); // copy from pc
}

You need to use memcpy so copy all of the contents of the array and not just assign the pointer.

This site is a great resource to see how to properly do it: http://icu-project.org/docs/papers/cpp_rep...t_operator.html

Finally you might want to consider using std::vector<BYTE>, as that will massively simplify the code, you can also use it as an array &vector[0] is equivalent to just using a pointer to an array of BYTEs. You'll want to use vector::reserve to make sure you have a large enough vector to contain all the bytes, and vector::size will give you the equivalent of nBufferSize.

Edited by Lant
Link to comment
Share on other sites

  • 0

When a class owns resources that are not automatically freed, like heap memory, it's best to avoid copying it entirely. It is possible to implement a correct copy constructor, but it requires very careful design. An easy way to enforce non-copyable semantics is declaring the copy and assignment constructors private, as described in Effective C++.

Link to comment
Share on other sites

  • 0
When a class owns resources that are not automatically freed, like heap memory, it's best to avoid copying it entirely.

Wait, so that means most STL containers should not be copyable? I'd say whether you want copy semantics depend on how that class will be used.

If you do want this class to not be copyable look into boost::noncopyable, its a simple and clean way to make a class noncopyable

Link to comment
Share on other sites

  • 0
Wait, so that means most STL containers should not be copyable?
They should be copyable because they can contain anything, including copyable objects.
I'd say whether you want copy semantics depend on how that class will be used.
Yes, and specifically it depends on the ownership semantics for that class. Copying means transferring ownership in some way, which doesn't necessarily make sense. In general, any RAII class should not be copyable.
Link to comment
Share on other sites

This topic is now closed to further replies.
  • Recently Browsing   0 members

    • No registered users viewing this page.