Removing warnings from the Bullet/src library

[Erwin Coumans]

Martijn Reuvers provided a patch that removes all warnings, and it improves the library in several other ways.

It also splits folder structure in include and src, and changes all #include directives from

Code: Select all

#include "LinearMath/btAabbUtil2.h"
#include "btDispatcher.h"

to

Code: Select all

#include <bullet/LinearMath/btAabbUtil2.h>
#include <bullet/BulletCollision/BroadphaseCollision/btDispatcher.h>

Convention in Bullet is to not include a full path if the headerfile exists in exactly the same folder as the .cpp file. Otherwise the src is assume to be the include path. This change/split into include/src disrupts the development too much (it breaks all build systems), so it is better to just focus on the warnings.

It will take a while before processing/reviewing all proposed changes. Here some early feedback:

- lots and lots of signed/unsigned problems. They have been replaced by
static_casts

Might be better to review why the unsigned int/int warning happened. In many cases we might get rid of the warning by using a signed int. For example:

Code: Select all

int hash = static_cast<int>(getHash(static_cast<unsigned int>(proxyId1), static_cast<unsigned int>(proxyId2)) & (m_overlappingPairArray.capacity()-1));

Instead of the static_cast, the getHash should return a signed int, and use a signed int as arguments.

- uninitialized variables

As long as it doesn't hurt performance. For example btVector3 constructor doesn't initialize its elements, and this is not necessary, so we shouldn't need to add that.

- unreferenced variables

We rather remove the variable name from the implementation and just leaving the type, instead of adding (void*) everywhere.

- unused local functions
- constant conditional expressions
- etc etc

Code: Select all

void ReleaseHull(PHullResult &result);
void ReleaseHull(PHullResult &result)
{
}

This seems extremely verbose, what kind of warning requires a separate definition+declaration?

Thanks for the patch,
Erwin

[mreuvers]

Convention in Bullet is to not include a full path if the headerfile exists in exactly the same folder as the .cpp file. Otherwise the src is assume to be the include path. This change/split into include/src disrupts the development too much (it breaks all build systems), so it is better to just focus on the warnings.

No problem. Just in case anyone wants to have seperated source and headers, this should be the way to go.

Instead of the static_cast, the getHash should return a signed int, and use a signed int as arguments.

Absolutely correct. However I didn't want to change ANY functionality in the code, so I fixed the warnings by adding static_casts it would be much better to add signed/unsigned consistency. For example sizes should be defined by a std::size_t type, not by int or unsigned int.

uninitialized variables
As long as it doesn't hurt performance. For example btVector3 constructor doesn't initialize its elements, and this is not necessary, so we shouldn't need to add that.

This shouldn't hurt performance at all. In fact the compiler only complains about uninitialized variables if there is a possible code execution path that doesn't initialize the variables. In that case you really SHOULD initialize the variables or there will be random data in them, which can open a whole can of worms.

We rather remove the variable name from the implementation and just leaving the type, instead of adding (void*) everywhere.

Again, I didn't want to change the code too much, so I simply voided them. Do note that it isn't always possible to remove the unused references, simply because there might be a difference between release-mode and debug-mode.

Consider this example:

Code: Select all

void foo(int p_bar)
{
  ASSERT(p_bar >= 0);
}

Asserts are disabled in release mode, so the compiler will complain about a unused variable. To fix this you must change this to

Code: Select all

void foo(int p_bar)
{
  (void)p_bar;
  ASSERT(p_bar >= 0);
}

Or add ugly #ifdef _DEBUG pragmas around it, something I really wouldn't prefer as it tends to clutter the code.

function prototypes

This seems extremely verbose, what kind of warning requires a separate definition+declaration?

The highest level warning. I believe it is part of the C++ standard to have function prototypes. Microsoft compilers don't follow standards by nature, so they don't complain about it. But pretty much any other compiler I know does complain about it in -Wall mode.

Cheers,


Martijn Reuvers
Two Tribes B.V.
http://www.twotribes.com

[Erwin Coumans]

Ok, that makes sense.

Going through the patch, I noticed a change from quickSort to heapSort in btUnionFind.cpp
from

Code: Select all

	  m_elements.quickSort(btUnionFindElementSortPredicate());

to

Code: Select all

	//perhaps use radix sort?
	  m_elements.heapSort(btUnionFindElementSortPredicate());

Was this change intentional?
Thanks,
Erwin

[mreuvers]

This change/split into include/src disrupts the development too much (it breaks all build systems), so it is better to just focus on the warnings.

You actually can use the <> solution in all older projects: simply copy all .h files to the source directories again and add your source directory (./src) to the include path. This way people can get to choose where to put the .h files. Or is this not preferred?

[Erwin Coumans]

The patch has been applied, without the include changes (split/directives).

http://code.google.com/p/bullet/source/detail?r=1156
http://code.google.com/p/bullet/issues/detail?id=56

Warnings in the external optional libraries (COLLADA_DOM, libxml) are suppressed now, so core Bullet libraries (Bullet/src) are more visible.
Thanks again,
Erwin

[mreuvers]

Was this change intentional?
Thanks,
Erwin

To my knowledge I didn't change that. It was already in the commit (1155) I downloaded.

[chunky]

Coo. Almost all of the warnings are gone now when I try to compile 2.69 on OSX and linux. There's a diff to fix the rest of them here [basically, a couple of signed/unsigned compares, and a few case statements without a default or all the possible cases]:
http://chunkyks.com/bullet-2.69-warnings.diff

Actually that misses two, the files BulletSoftBody/btSoftBody.cpp and BulletSoftBody/btSoftBodyHelpers.cpp both need a newline appended at the end of the file for them to compiler completely without warnings on the gcc versions I'm using here. I had to use the -b option to diff, though, so it skipped those in the diff, but if you could just add those by hand, it'd be perfect.

Thanks,
Gary (-;

[Erwin Coumans]

Thanks, that will be the first contribution for Bullet 2.70!

Erwin