Help needed with Class(const Class& cpy) and Class& operator=(const Class& cpy)

[Paul Belikian]

Hello All,

I am having problems with the operator= and copy constructors in the following
class. I know since I have a pointer as a class member, I need to allocate more
memory so that these pointers don't point to the same address in memory. I'm using
the full class in a CArray, and for the most part it functions. It fails when
the destructor is called because I'm deleteing memory I shouldn't. Can some one
please provide me with the correct way of creating the copy constructor and the
operator= for this class? Or is there a better way to do this???

TIA!


The (basic) header file....

class CUdpPacket : public CObject
{
public:

CUdpPacket(WORD wBufferSize, BYTE *pBuffer);
CUdpPacket();
// copy operator...
CUdpPacket(const CUdpPacket& cpy);
// assignment constructor...
CUdpPacket& operator=(const CUdpPacket& cpy);

virtual ~CUdpPacket();

BYTE *m_pData;
WORD m_BufferSize;
};




The (basic) cpp file....

CUdpPacket::CUdpPacket()
{
if(m_pData)
{
delete m_pData;
m_pData = 0;
}
m_BufferSize = 0;
TRACE("Default Constructor called.\n");
}

CUdpPacket::CUdpPacket(WORD wBufferSize, BYTE *pBuffer)
{
m_BufferSize = wBufferSize;
m_pData = new BYTE[m_BufferSize];
TRACE("Extended Constructor called.\n");
}

// copy constructor...
CUdpPacket::CUdpPacket(const CUdpPacket& cpy)
{
delete m_pData;
m_pData = 0;
m_BufferSize = cpy.m_BufferSize;
m_pData = new BYTE[m_BufferSize];
*m_pData = * cpy.m_pData;
TRACE("Copy Constructor called.\n");
}

// assignment operator
CUdpPacket& CUdpPacket:perator=(const CUdpPacket& cpy)
{
if(this == &cpy)
return *this;

delete m_pData;
m_pData = NULL;
m_BufferSize = cpy.m_BufferSize;
m_pData = new BYTE[m_BufferSize];
*m_pData = * cpy.m_pData;
TRACE("operator= called.\n");
return *this;
}

CUdpPacket::~CUdpPacket()
{
if(m_pData)
{
delete m_pData;
m_pData = 0;
}
m_BufferSize = 0;
TRACE("Destructor called.\n");
}




Regards,

Paul Belikian

[Gregory64]

Hi,
I've seen three problems in your code


CUdpPacket& CUdpPacket:perator=(const CUdpPacket& cpy)
{
if(this == &cpy)
return *this;
//delete m_pData; first one. if you allocate new[] you have
// to use []delete also you have to check for m_pData NULL,
// The object may be created by default constractor
if(m_pData)
delete[] m_pData;
m_pData = NULL;//not nessesery
m_BufferSize = cpy.m_BufferSize;
m_pData = new BYTE[m_BufferSize];
//*m_pData = * cpy.m_pData; second one. You copy just one byte not all pool
memmove(m_pData,cpy.m_pData,m_BufferSize);
TRACE("operator= called.\n");
return *this;
}

// copy constructor...
CUdpPacket::CUdpPacket(const CUdpPacket& cpy)
{
delete m_pData;// here is another problm, what are you deleting?
//there is no memeory alloceted for m_pData yet
m_pData = 0;
m_BufferSize = cpy.m_BufferSize;
m_pData = new BYTE[m_BufferSize];
*m_pData = * cpy.m_pData;// the same
TRACE("Copy Constructor called.\n");
}

//I suggest
// copy constructor...
CUdpPacket::CUdpPacket(const CUdpPacket& cpy)
{

m_pData = 0;
*this=cpy//using our operator= to do the same work
TRACE("Copy Constructor called.\n");
}
//also change delete m_pData in destr. to delete[] m_pData

[Paul Belikian]

Hi There,

Thanks for your help...it worked perfectly. The comments in your code will help
me to figure out why it works, but thank you for taking the time and assisting.
The example class I posted was just that, an example. But the pointer confusion
was the most important part; now the 'full blown' class works the way it should
...without any leaks or asserts. Your copy and operator= 'shortcuts' reduced a lot
of the manual coding that would have been needed in the final class.

Thanks for all your help!



Regards,

Paul Belikian

Gregory64s answers were generally correct apart from:


//also you have to check for m_pData NULL [before calling delete]




You never have to check if a pointer is NULL before calling delete. C++ copes with delete being called on NULL pointers just fine.

[Paul Belikian]

Hi,

I think it's more of making sure the pointer is pointing to allocated memory rather than not. I like checking for NULL so that I don't have to call delete if it is NULL.

Thanks,



Regards,

Paul Belikian

[jdala]

the common practice, is to assign a pointer to NULL for safe computing. i agree that delete will do nothing if a pointer is already NULL, but in my
stand, it is better to check first if a pointer is
NOT NULL. This is safe because, somewhere in the code, the pointer had been deleted already.

regards,
YEOJ