favreau / bullet

Automatically exported from code.google.com/p/bullet
0 stars 0 forks source link

Acceptable code patch? #535

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
I inherited some patches while trying to create a Fedora package of Blender and 
was told that I should get the attached patch reviewed for  acceptability.

Thanks,
Richard

Original issue reported on code.google.com by hobbes1...@gmail.com on 7 Sep 2011 at 12:39

Attachments:

GoogleCodeExporter commented 9 years ago

+   SIMD_FORCE_INLINE btVector3() {
+   m_floats[0]=m_floats[1]=m_floats[2]=m_floats[3]=btScalar(0.);}

The btVector3 is not initialized by default, by choice, for optimization 
reasons. If you require an initialized btVector3, please use the other 
constructor.

Does Fedora not compile with this constructor? Can you provide the warnings?

The other part of the patch, can we change it into the better readable version:

//pad the structure size to 64 bytes total
char    m_padding[20];

Original comment by erwin.coumans on 12 Sep 2011 at 10:26

GoogleCodeExporter commented 9 years ago
I have no idea if it solves any warnings or not, as I mentioned, the patch is 
inherited and there were no comments in the spec file describing why it was 
thought to be needed. I'll try dropping it and see what happens.

Thanks,
Richard

Original comment by hobbes1...@gmail.com on 13 Sep 2011 at 1:18

GoogleCodeExporter commented 9 years ago

char    m_padding[20]; has been applied in

http://code.google.com/p/bullet/source/detail?r=2420

Original comment by erwin.coumans on 13 Sep 2011 at 1:53

GoogleCodeExporter commented 9 years ago
Ok I replaced my patch with just the "char m_padding[20];" adjustment and I'm 
attaching the output of grep'ing for "warning" and "bullet". If you need 
additional context, let me know which lines you're interested in and I'll pull 
a larger build log segment.

Original comment by hobbes1...@gmail.com on 13 Sep 2011 at 2:07

Attachments:

GoogleCodeExporter commented 9 years ago
The "char m_padding[20];"  was already applied.

Is there a policy in Fedora to reject code that contains warnings?

Original comment by erwin.coumans on 13 Sep 2011 at 7:14

GoogleCodeExporter commented 9 years ago
fixed the padding issue, not the btVector3 constructor initialization

Original comment by erwin.coumans on 13 Sep 2011 at 7:35

GoogleCodeExporter commented 9 years ago
Sorry, I' should have mentioned. I'm building from the bullet built into the 
blender package so I'll need to apply the patch until they update their version 
(that doesn't happen automatically right?)

There's no policy against accepting software with build warnings, but in 
general less warnings are better :) In this case since the action is 
intentional, I wonder if I could modify the build flags only for btVector3 to 
quiet the warnings.

Original comment by hobbes1...@gmail.com on 13 Sep 2011 at 8:52