swordlegend / recastnavigation

Automatically exported from code.google.com/p/recastnavigation
zlib License
0 stars 0 forks source link

rcScopedDelete could fail #93

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
This is the current state of rcScopedDelete, a helper to delete arrays in scope:
template<class T> class rcScopedDelete
{
    T* ptr;
public:
    inline rcScopedDelete() : ptr(0) {}
    inline rcScopedDelete(T* p) : ptr(p) {}
    inline ~rcScopedDelete() { delete [] ptr; }
    inline operator T*() { return ptr; }
    inline T* operator=(T* p) { ptr = p; return ptr; }
};

There are two problems with that code:
The assignment operator doesn't delete the old pointer. You need to add a 
delete[] ptr; there, or you will get a memory leak on each assignment.

You need to write a correct copy constructor, since a faulty one is generated 
automatically. If you copied such an object, you would have the same raw 
pointer in two rcScopedDelete, and you will delete the same pointer twice. The 
solution depends on what you want to do. Either disallow copy construction by 
defining a private one, or add a reference counting mechanism to it to make it 
a shared pointer.

If you want to keep using C++, you could also ust Boost's smart pointers, which 
are ready to use and robust.

Original issue reported on code.google.com by j.d...@gmx.de on 1 Jul 2010 at 9:07

GoogleCodeExporter commented 9 years ago
It probably would also be better to rename the class, so that everyone can see 
that it always handles arrays, not simple classes. I know, there is a comment 
for that, but you never know.
If you decided to use that class for a simple object, you would call the wrong 
delete operator on it, which produces undefined behavior.

Original comment by j.d...@gmx.de on 1 Jul 2010 at 9:10

GoogleCodeExporter commented 9 years ago
The scoped delete is only used to delete temporary buffers in some functions. I 
wanted to use "goto failure", but it does not play nicely with all compilers. 
There is no way I'm going to add dependency to boost.

I removed the assignment operator and moved the class def to another header 
which is not usually visible and commented it being for internal use (R177).

Original comment by memono...@gmail.com on 9 Jul 2010 at 1:02