Irritating memory handling

[jhughes]

Hi!

I ran into a snag today while doing some quick and dirty tests (unrelated to Bullet).

btCompoundShape, and possibly others, have naked pointers as members (m_dynamicAabbTree, specifically). Since the destructor does not use any kind of smart pointer to determine if the object was copied somehow, it simply deletes the memory if it was allocated during its lifetime. Which means the following line will have a memory leak AND crash with a double-delete, every time:

{
MyObject o;
o = MyObject o2;
} // crash

In the above example, MyObject has a btCompoundShape as a member. If I create another one and assign from one to the other, the temporary 'o2' will go out of scope and delete the m_dynamicAabbTree, which is now copied into 'o'. However, the pointer that 'o' originally had is leaked, because nobody deleted it during the assignment.

The above is a short, contrived example, but the situation exists in another situation. My proposal(s) for fixing such issues, in order of preference:
1) use a lightweight smart pointer that properly shares data that is impractical to deep copy and allow them to go out of scope cleanly
2) allow the user to perform the allocation and pass it in, and manage the lifetime of the pointer manually, so it's clearly something the user needs to take care of
3) disable copy and assignment entirely

Memory management is a pain in C++, but there are definitely steps than can be taken that allow the compiler to help without impacting performance.

Thanks for a great physics engine!
JH

[Erwin Coumans]

Bullet assumes you keep a pointer to shapes/objects. You will have issues if you embed objects and use the copy/assignment operator, so we should go for option 3, but haven't enforced this yet. For now, it is a responsibility for the developer.

3) disable copy and assignment entirely

Thanks,
Erwin