Pointer Best Practice's

[Philip Lee Bridson]

Hi Guys,

Just a quick one really. In a basic class defined as below:

template<class T>
class Foo
{
public:
// construct, copy, destroy...
// not needed in this example...

// accessors...
T* Get() const;

private:
T* m_value;
};

Is it best to implement the accessor as this:

T* Foo::Get() const
{
return m_value;
}

or as this...?

T* Foo::Get() const
{
return new T(m_value);
}

The only reason I ask is because the first method enables the user to do something like this: (ignore the use of T out of context...)

Foo* foo = new Foo<T>();
T* temp = foo->Get();
delete temp; // deletes m_value causing error later... 

But every where I have been looking on the web says to do it that way. Plus my company says to do it that way because there is a higher likely-hood of memory leaks using the second method. Which is best?

I appreciate any help offered.

Regards,

Phil.

[helios]

An accessor should never do dynamic allocations. People just don't expect them to, so they're unlikely to remember to free the return value. If you need to dynamically allocate a copy, just don't call the function a variation of "get_x". Call it something like "copy_x". As long as the name makes it clear that the function is making a copy, there's no problem. You can later explain the documentation how the copy is made.

There's two ways to solve your problem. Either return a const T *, which I believe the compiler won't let you delete (without casting to 'T *', which has undefined behavior and you can't prevent, anyway. If the users want to shoot themselves in the foot, who are you to stop them?), or return T. That is, exactly like your third snippet, only remove the asterisk on line 1 and change line 3 to return T(*m_value);. Depending on what T is, this may or may not be a good idea.

[Philip Lee Bridson]

Thank you - your answer was very helpful.

Regards,

Phil.