Re: David Adam : d3dx8: Implement D3DXQuaternionInverse.

[Robert Shearman]

Alexandre Julliard wrote:
> /*_________________D3DXQUATERNION________________*/
>
> -D3DXQUATERNION* WINAPI D3DXQuaternionMultiply(D3DXQUATERNION * pout, CONST D3DXQUATERNION *pq1, CONST D3DXQUATERNION * pq2)
> +D3DXQUATERNION* WINAPI D3DXQuaternionInverse(D3DXQUATERNION *pout, CONST D3DXQUATERNION *pq)
> +{
> - D3DXQUATERNION temp;
> - FLOAT norm;
> -
> - norm = D3DXQuaternionLengthSq(pq);
> - if ( !norm )
> - {
> -
> pout->x = 0.0f;
> -
> pout->y = 0.0f;
> -
> pout->z = 0.0f;
> -
> pout->w = 0.0f;
> - }
> - else
> - {
> - D3DXQuaternionConjugate(&temp, pq);
> - pout->x = temp.x / norm;
> - pout->y = temp.y / norm;
> - pout->z = temp.z / norm;
> - pout->w = temp.w / norm;
> - }
> - return pout;
>
> +}
> -
>
> +D3DXQUATERNION* WINAPI D3DXQuaternionMultiply(D3DXQUATERNION *pout, CONST D3DXQUATERNION *pq1, CONST D3DXQUATERNION *pq2)
>  {
>      pout->x = pq2->w * pq1->x + pq2->x * pq1->w + pq2->y * pq1->z - pq2->z * pq1->y;
>      pout->y = pq2->w * pq1->y - pq2->x * pq1->z + pq2->y * pq1->w + pq2->z * pq1->x;

This change causes the following warnings for me:
math.c: In function ‘D3DXQuaternionInverse’:
math.c:695: warning: ‘temp.x’ may be used uninitialised in this function
math.c:695: warning: ‘temp.y’ may be used uninitialised in this function
math.c:695: warning: ‘temp.z’ may be used uninitialised in this function
math.c:695: warning: ‘temp.w’ may be used uninitialised in this function

$ gcc --version
gcc (GCC) 4.0.3 (Ubuntu 4.0.3-1ubuntu5)
Copyright (C) 2006 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

I believe that this is causes by D3DXQuaternionConjugate not writing to 
temp in one case (pq being NULL) and the compiler not recognising that 
this case is already covered by the !norm branch above, since 
D3DXQuaternionLengthSq will return 0.0f if pq is NULL.

The attached patch works around the warning. However, there are 
currently no tests for the case of pq being NULL so I don't know whether 
it is correct.

David Adam : d3dx8: Implement D3DXQuaternionInverse.

Robert Shearman  a écrit :
> Alexandre Julliard wrote:
> > /*_________________D3DXQUATERNION________________*/
> > -D3DXQUATERNION* WINAPI D3DXQuaternionMultiply(D3DXQUATERNION *  
> > pout, CONST D3DXQUATERNION *pq1, CONST D3DXQUATERNION * pq2)
> > +D3DXQUATERNION* WINAPI D3DXQuaternionInverse(D3DXQUATERNION *pout,  
> > CONST D3DXQUATERNION *pq)
> > +{
> > - D3DXQUATERNION temp;
> > - FLOAT norm;
> > -
> > - norm = D3DXQuaternionLengthSq(pq);
> > - if ( !norm )
> > - {
> > -
> > pout->x = 0.0f;
> > -
> > pout->y = 0.0f;
> > -
> > pout->z = 0.0f;
> > -
> > pout->w = 0.0f;
> > - }
> > - else
> > - {
> > - D3DXQuaternionConjugate(&temp, pq);
> > - pout->x = temp.x / norm;
> > - pout->y = temp.y / norm;
> > - pout->z = temp.z / norm;
> > - pout->w = temp.w / norm;
> > - }
> > - return pout;
> >
> > +}
> > -
> >
> > +D3DXQUATERNION* WINAPI D3DXQuaternionMultiply(D3DXQUATERNION  
> > *pout, CONST D3DXQUATERNION *pq1, CONST D3DXQUATERNION *pq2)
> > {
> >     pout->x = pq2->w * pq1->x + pq2->x * pq1->w + pq2->y * pq1->z -  
> > pq2->z * pq1->y;
> >     pout->y = pq2->w * pq1->y - pq2->x * pq1->z + pq2->y * pq1->w +  
> > pq2->z * pq1->x;
>
> This change causes the following warnings for me:
> math.c: In function ‘D3DXQuaternionInverse’:
> math.c:695: warning: ‘temp.x’ may be used uninitialised in this function
> math.c:695: warning: ‘temp.y’ may be used uninitialised in this function
> math.c:695: warning: ‘temp.z’ may be used uninitialised in this function
> math.c:695: warning: ‘temp.w’ may be used uninitialised in this function
>
> $ gcc --version
> gcc (GCC) 4.0.3 (Ubuntu 4.0.3-1ubuntu5)
> Copyright (C) 2006 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>
> I believe that this is causes by D3DXQuaternionConjugate not writing  
> to temp in one case (pq being NULL) and the compiler not recognising  
> that this case is already covered by the !norm branch above, since  
> D3DXQuaternionLengthSq will return 0.0f if pq is NULL.
>
> The attached patch works around the warning. However, there are  
> currently no tests for the case of pq being NULL so I don't know  
> whether it is correct.
>
> -- 
> Rob Shearman

I have just tried with a native d3dx8 dll. Giving a null pointer to pq  
leads to a crash. So, the application must take care to not send such  
a pointer.
So, I think that to avoid the warning that you see, we could just  
initialize the variable temp before the if condition.

David