Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should prefer IV when both IV and NV are OK #39

Open
vphantom opened this issue Jan 11, 2018 · 1 comment
Open

Should prefer IV when both IV and NV are OK #39

vphantom opened this issue Jan 11, 2018 · 1 comment

Comments

@vphantom
Copy link

Hello,

After a lot of digging, I finally managed to understand why this module in my test script was always assuming that integers were huge float64's. Here's my discovery:

use Devel::Peek;
use Data::MessagePack;
my $mp = new Data::MessagePack;
$mp->prefer_integer(1);

my $a = [ 30000, 123, 300, 1000, 1000, 500, 500 ];

print 'Clean: ', unpack('H*', $mp->pack($a)), "\n";
#print Dump($a);
my $b = pack('wwwwwww', @{ $a });
#print Dump($a);
print 'After pack(): ', unpack('H*', $mp->pack($a)), "\n";

This outputs:

Clean: 97cd75307bcd012ccd03e8cd03e8cd01f4cd01f4
After pack(): 97cb40dd4c0000000000cb405ec00000000000cb4072c00000000000cb408f400000000000cb408f400000000000cb407f400000000000cb407f400000000000

Using Devel::Peek's Dump() I finally figured out that pack() does its reading in a way which triggers Perl to add an NV in addition to the original IV. Before pack() the first value is:

SV = IV(0x17d32b0) at 0x17d32c0
  REFCNT = 1
  FLAGS = (PADMY,IOK,pIOK)
  IV = 30000

But after pack() reads it, it becomes:

SV = PVNV(0x1934710) at 0x17d32c0
  REFCNT = 1
  FLAGS = (PADMY,IOK,NOK,pIOK,pNOK)
  IV = 30000
  NV = 30000
  PV = 0

My understanding is that the integer version of the value is still usable, but MessagePack prioritizes the float version. My first attempt at fixing that (PP.pm#274 and pack.c#193) broke test 21 "dirty float" so there seems to be more to Perl's IOK/NOK than meets my eye.

I'll experiment further and hopefully come up with a pull request. I figure there must be an efficient way to determine whether the IV loses precision vs the NV. If it doesn't then IV wins, if it does then NV wins.

@vphantom
Copy link
Author

I was able to make PP.pm pass test 21 by moving the B::SVp_NOK test below the B::SVp_IOK test, and augmenting the latter with a simple sanity check:

--- PP-orig.pm	2018-01-11 10:58:21.374129040 -0500
+++ PP.pm	2018-01-11 10:13:01.782109956 -0500
@@ -271,10 +271,7 @@
         return $header . $value;
 
     }
-    elsif( $flags & B::SVp_NOK ) { # double only
-        return pack_double( $value );
-    }
-    elsif ( $flags & B::SVp_IOK ) {
+    elsif ( $flags & B::SVp_IOK && $value == int($value) ) {
         if ($value >= 0) { # UV
             return    $value <= 127 ?    CORE::pack 'C',        $value
                     : $value < 2 **  8 ? CORE::pack 'CC', 0xcc, $value
@@ -290,6 +287,9 @@
                     : pack_int64( $value );
         }
     }
+    elsif( $flags & B::SVp_NOK ) { # double only
+        return pack_double( $value );
+    }
     else {
         _unexpected("data type %s", $b_obj);
     }

However, I am stumped as to how I could achieve the same result in pack.c, so this isn't worthy of a pull request. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant