Storing a pass-by-reference parameter as a pointer - Bad practice?
- by Karl Nicoll
I recently came across the following pattern in an API I've been forced to use:
class SomeObject
{
public:
// Constructor.
SomeObject(bool copy = false);
// Set a value.
void SetValue(const ComplexType &value);
private:
bool m_copy;
ComplexType *m_pComplexType;
ComplexType m_complexType;
};
// ------------------------------------------------------------
SomeObject::SomeObject(bool copy) :
m_copy(copy)
{
}
// ------------------------------------------------------------
void SomeObject::SetValue(const ComplexType &value)
{
if (m_copy)
m_complexType.assign(value);
else
m_pComplexType = const_cast<ComplexType *>(&value);
}
The background behind this pattern is that it is used to hold data prior to it being encoded and sent to a TCP socket. The copy weirdness is designed to make the class SomeObject efficient by only holding a pointer to the object until it needs to be encoded, but also provide the option to copy values if the lifetime of the SomeObject exceeds the lifetime of a ComplexType.
However, consider the following:
SomeObject SomeFunction()
{
ComplexType complexTypeInstance(1); // Create an instance of ComplexType.
SomeObject encodeHelper;
encodeHelper.SetValue(complexTypeInstance); // Okay.
return encodeHelper;
// Uh oh! complexTypeInstance has been destroyed, and
// now encoding will venture into the realm of undefined
// behaviour!
}
I tripped over this because I used the default constructor, and this resulted in messages being encoded as blank (through a fluke of undefined behaviour). It took an absolute age to pinpoint the cause!
Anyway, is this a standard pattern for something like this? Are there any advantages to doing it this way vs overloading the SetValue method to accept a pointer that I'm missing?
Thanks!
