Suspect code in magick/floats.c

--=-=-=


I just switched from a 32 bit machine to a 64 bit one, and this file
stopped compiling for me.  gcc complains about these lines:

MagickExport
int _Gm_convert_fp16_to_fp32 (const fp_16bits *fp16, float *fp32)
{
...
  if ((int)*fp16 != 0)

and also

MagickExport
int _Gm_convert_fp24_to_fp32 (const fp_24bits *fp24, float *fp32, const int mode)
{
...
  if ((int)*fp24 == 0)

I do not think these tests do what the author intended.  Here's a stupid
little test program I wrote to experiment:

#include <stdio.h>

typedef unsigned char a16[2];

void printit(const a16* p)
{
    printf("The answer is %d\n", (int)*p);
}

int main(void)
{
    unsigned char a32[4] = {0, 0, 0, 0};

    printit(a32);
}

This compiles only because I switch off the warnings-as-errors flag, and
the answer seems to be random - my guess is it is the bit pattern in the
pointer that gets printed, not what it points to.

In any case I hope you agree that the code is obscure at best and should
be rewritten with memcmp() or something.  It also seems to be just an
optimization, since the "not zero" branch following the test does the
right thing even if the source _is_ zero.  I simply removed the tests
(see attached patch), then it compiles cleanly and all tests still pass.


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline; filename=floats.diff
Content-Description: remove tests for zero source in magick/floats.c

diff --git a/magick/floats.c b/magick/floats.c
index 1dc06ee..5808513 100644
--- a/magick/floats.c
+++ b/magick/floats.c
@@ -58,29 +58,27 @@ int _Gm_convert_fp16_to_fp32 (const fp_16bits *fp16, float *fp32)
   new_m3 = new_m2 = new_m1 = m2 = m1 = 0;
 
   little_endian = *((unsigned char *)&little_endian) & '1';
-  if ((int)*fp16 != 0)
+  if (little_endian)
     {
-      if (little_endian)
-	{
-	  sbit =  *(src + 1) & 0x80;
-	  expt = (*(src + 1) & 0x7F) >> 2;
-	  m2 =    *(src + 1) & 0x03;
-	  m1 = *src;
-	}
-      else
-	{
-	  sbit =  *src & 0x80;
-	  expt = (*src & 0x7F) >> 2;
-	  m2 =    *src & 0x03;
-	  m1 =  *(src + 1);
-	}
-
-      if (expt != 0)
-	new_expt = expt - 15 + 127;
-      new_m3  =  (m2 << 5) | (m1 & 0xF8) >> 3; 
-      new_m2  =  (m1 & 7) << 5;
-      new_m1  = 0;
+      sbit =  *(src + 1) & 0x80;
+      expt = (*(src + 1) & 0x7F) >> 2;
+      m2 =    *(src + 1) & 0x03;
+      m1 = *src;
+    }
+  else
+    {
+      sbit =  *src & 0x80;
+      expt = (*src & 0x7F) >> 2;
+      m2 =    *src & 0x03;
+      m1 =  *(src + 1);
     }
+
+  if (expt != 0)
+      new_expt = expt - 15 + 127;
+  new_m3  =  (m2 << 5) | (m1 & 0xF8) >> 3; 
+  new_m2  =  (m1 & 7) << 5;
+  new_m1  = 0;
+
   if (little_endian)
     {
       *dst = new_m1;
@@ -504,37 +502,28 @@ int _Gm_convert_fp24_to_fp32 (const fp_24bits *fp24, float *fp32, const int mode
   /* new_mant = mant = 0; */
   new_m3 = new_m2 = new_m1 = m2 = m1 = 0;
 
-  if ((int)*fp24 == 0)
+  if (little_endian)
     {
-      *dst = 0;
-      *(dst + 1) = 0;
-      *(dst + 2) = 0;
-      *(dst + 3) = 0;
+      sbit = *(src + 2) & 0x80;
+      expt = *(src + 2) & 0x7F;
+      m2  =  *(src + 1);
+      m1  =  *src;
     }
   else
     {
-      if (little_endian)
-	{
-	  sbit = *(src + 2) & 0x80;
-	  expt = *(src + 2) & 0x7F;
-	  m2  =  *(src + 1);
-	  m1  =  *src;
-	}
-      else
-	{
-	  sbit = *src & 0x80;
-	  expt = *src & 0x7F;
-	  m2  =  *(src + 1);
-	  m1  =  *(src + 2);
-	}
-
-      if (expt != 0)
-	new_expt = expt - 63 + 127;
-      /* mant = (m2 << 8) | m1; */
-      new_m3  =  (m2 & 0xFE) >> 1;
-      new_m2  = ((m2 & 0x01) << 7) | ((m1 & 0xFE) >> 1);
-      new_m1  =  (m1 & 0x01) << 7;
+      sbit = *src & 0x80;
+      expt = *src & 0x7F;
+      m2  =  *(src + 1);
+      m1  =  *(src + 2);
     }
+
+  if (expt != 0)
+      new_expt = expt - 63 + 127;
+  /* mant = (m2 << 8) | m1; */
+  new_m3  =  (m2 & 0xFE) >> 1;
+  new_m2  = ((m2 & 0x01) << 7) | ((m1 & 0xFE) >> 1);
+  new_m1  =  (m1 & 0x01) << 7;
+
   /* We do not have to worry about underflow or overflow 
    * since the target has more significant bits in the
    * exponent and the significand.

--=-=-=



-- 
Ian Zimmerman
gpg public key: 1024D/C6FF61AD
fingerprint: 66DC D68F 5C1B 4D71 2EE5  BD03 8A00 786C C6FF 61AD
http://www.gravatar.com/avatar/c66875cda51109f76c6312f4d4743d1e.png
Rule 420: All persons more than eight miles high to leave the court.

--=-=-=
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

------------------------------------------------------------------------------
WINDOWS 8 is here. 
Millions of people.  Your app in 30 days.
Visit The Windows 8 Center at Sourceforge for all your go to resources.
http://windows8center.sourceforge.net/
join-generation-app-and-make-money-coding-fast/
--=-=-=
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Graphicsmagick-tools mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/graphicsmagick-tools

--=-=-=--