bug in btAlignedObjectArray.h copy constructor

[mreuvers]

Hi there,

We discovered a serious bug in the latest btAlignedObjectArray.h file. Recently the copy constructor was added to it, however it simply ignores the BT_USE_PLACEMENT_NEW settings. This results in bizarre random stack crashes.

You should simply use the copy method there, which I made const (since it is const anyhow). Less code duplication, less bugs, more fun

Patch below.

Cheers

Code: Select all

Index: C:/svn/ttdev/libs/shared/inc/bullet/LinearMath/btAlignedObjectArray.h
===================================================================
--- C:/svn/ttdev/libs/shared/inc/bullet/LinearMath/btAlignedObjectArray.h	(revision 2960)
+++ C:/svn/ttdev/libs/shared/inc/bullet/LinearMath/btAlignedObjectArray.h	(revision 2961)
@@ -58,7 +58,7 @@
 		{
 			return (size ? size*2 : 1);
 		}
-		SIMD_FORCE_INLINE	void	copy(int start,int end, T* dest)
+		SIMD_FORCE_INLINE	void	copy(int start,int end, T* dest) const
 		{
 			int i;
 			for (i=start;i<end;++i)
@@ -123,14 +123,9 @@
 		btAlignedObjectArray(const btAlignedObjectArray& otherArray)
 		{
 			init();
-
 			int otherSize = otherArray.size();
 			resize (otherSize);
-			int i;
-			for (i=0;i<otherSize;i++)
-			{
-				m_data[i] = otherArray[i];
-			}
+			otherArray.copy(0, otherSize, m_data);
 		}
 
 		SIMD_FORCE_INLINE	int capacity() const

[Erwin Coumans]

We applied the patch. Next time, can you please submit patches as an attachment to the Google Code issue tracker?
Generally it is not recommended to use this copy constructor. In most cases it is best to create a reference of an array, instead of duplicating it.

Code: Select all

typedef btAlignedObjectArray<btSomeClass> Array;

Array a;
Array b = a;//avoid this whenever possible

This is much better:
Array& b = a;//use a reference
const Array& b = a; //or const reference

Can you explain how you use the copy constructor?
Thanks,
Erwin

[mreuvers]

The thing is: we don't use the copy constructor. At least not that we know of. If we enable that original code, our Wii version will crash during the construction of the btSequentialImpulseConstraintSolver. We get garbled data on the stack, after btSequentialImpulseConstraintSolver has been created.

With the patch, we got no problems at all. Perhaps the compiler tries to be smart and adds the copy constructor in places you wouldn't expect? Which makes me wonder, if you shouldn't use it, why was it added in the first place?

[mreuvers]

Perhaps a good way to avoid people (or the compiler!) from mistakenly using this copy constructor, you should define it explicitly and not implement it:

Code: Select all

explicit btAlignedObjectArray(const btAlignedObjectArray& otherArray);

Now all code that calls this constructor, without you knowing it, will popup as compiler errors like this one:

Code: Select all

"no copy constructor available or copy constructor is declared 'explicit'	c:\svn\ttdev\libs\shared\inc\bullet\BulletCollision\CollisionDispatch\btGhostObject.h	77"

[Erwin Coumans]

Which makes me wonder, if you shouldn't use it, why was it added in the first place?

A developer recently came up with a bug, assigning an array which lead to corruption due to lack of copy constructor:
http://bulletphysics.com/Bullet/phpBB3/ ... 600#p11600

We could use 'explicit' indeed, but there might be legitimate reasons to use the copy constructor. For example, when copying an object that contains an array.

If developers suffer accidently use the copy constructor, and suffer performance loss they will find out during profiling.
Thanks for your patch, it should work fine now.
Erwin

[mreuvers]

We could use 'explicit' indeed, but there might be legitimate reasons to use the copy constructor. For example, when copying an object that contains an array.

If developers suffer accidently use the copy constructor, and suffer performance loss they will find out during profiling.

Sorry, but I disagree with you on this one. If you don't want to promote copy behavior because it's causing performance drops, I would make this explicit. A developer doesn't want to find out during profiling. It may be way too late and it'll cost him extra time to determine the cause etc, especially when the cause is the compiler 'trying to be smart'. It's not like you can't use it anymore once it's explicit: simply keep the implementation. Making it explicit only forces the programmer to carefully consider its usage, which is definitely the case here.

Just my 2 cts