Skip Menu |
 

This queue is for tickets about the Math-BigInt CPAN distribution.

Report information
The Basics
Id: 96329
Status: resolved
Priority: 0/
Queue: Math-BigInt

People
Owner: Nobody in particular
Requestors: jpeacock [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in:
  • 1.997
  • 1.9993
Fixed in: (no value)



Subject: () breaks non-foreign classes
Download (untitled) / with headers
text/plain 1.2k
Math::Currency is a fully functional Math::BigFloat class that handles the specific rounding rules that mathematical operations with currency requires. It has just come to my attention that a change in Math::BigInt: http://perl5.git.perl.org/perl.git/commitdiff/66a0495875e8130c45cac4fabd5f8d05f2f4c372 have broken certain operations due to changes in objectify(): https://rt.cpan.org/Ticket/Display.html?id=96254 In particular, your assumption that $class->as_float() should return an Math::BigFloat instance is the core of the problem. This method is undocumented in Math::BigInt/Float, but is used by Math::Currency to return a string representation of the correctly rounded value, not a Math::BigFloat object (as you evidently assumed) A simple change to Math::BigInt will DTRT: diff --git a/dist/Math-BigInt/lib/Math/BigInt.pm b/dist/Math-BigInt/lib/Math/BigInt.pm index 238d5f3..c2371ff 100644 --- a/dist/Math-BigInt/lib/Math/BigInt.pm +++ b/dist/Math-BigInt/lib/Math/BigInt.pm @@ -2673,7 +2673,7 @@ sub objectify { # If it is an object of the right class, all is fine. - if ($ref eq $a[0]) { + if ($ref->isa($a[0])) { next; } It is never a good idea to check for an exact class name; instead you should trust inheritance.
Download (untitled) / with headers
text/plain 403b
Speaking of good ideas, had you run "prove -l" in the .../dist/Math-BigInt directory, you would have seen that your "fix" causes more than 1000 tests to fail. I'll see what I can do, but it would be nice if you could provide a test case, preferably as simple as possible, that illustrates the problem. (I tried to install the latest Math::Currency, but some tests fail, so I'm not sure I can trust it.)
Subject: Re: [rt.cpan.org #96329] () breaks non-foreign classes
Date: Thu, 12 Jun 2014 20:47:04 -0400
To: bug-Math-BigInt [...] rt.cpan.org, jpeacock [...] cpan.org
From: John Peacock <john.peacock [...] havurah-software.org>
Download (untitled) / with headers
text/plain 662b
On 06/10/2014 08:40 AM, Peter John Acklam via RT wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=96329 > > > Speaking of good ideas, had you run "prove -l" in the .../dist/Math-BigInt directory, you would have seen that your "fix" causes more than 1000 tests to fail. > > I'll see what I can do, but it would be nice if you could provide a test case, preferably as simple as possible, that illustrates the problem. (I tried to install the latest Math::Currency, but some tests fail, so I'm not sure I can trust it.) >
You're right, that was very lazy of me. I'll try and produce two patches: 1) a test that fails and 2) a patch that fixes it. John
Download (untitled) / with headers
text/plain 540b
On Thu Jun 12 20:47:16 2014, john.peacock@havurah-software.org wrote: Show quoted text
> I'll try and produce two patches: 1) a test that fails and > 2) a patch that fixes it.
I have a fix. We must make sure $ref is defined, so the following slight modification works: if ($ref && $ref->isa($a[0])) { However, there are other problems with objectify(), so I will submit a patch that hopefully fixes them. Alas, objectify() is not well documented and does not have many tests, so it is too easy to break objectify() without it being caught by a test.
Download (untitled) / with headers
text/plain 498b
On Sat Jun 14 12:06:48 2014, pjacklam wrote: Show quoted text
> I have a fix. We must make sure $ref is defined, so the following > slight modification works: > > if ($ref && $ref->isa($a[0])) { >
I think the attached is better. Since you are already checking the undef case, reordering the tests will make sure that you are only trying to call ->isa() on an actual object. I haven't been able to get a test written (not enough time for hacking these days). I'll still try and work on it as I can... John
Subject: BigInt.diff
Download BigInt.diff
text/x-diff 1003b
diff --git a/dist/Math-BigInt/lib/Math/BigInt.pm b/dist/Math-BigInt/lib/Math/BigInt.pm index 238d5f3..d66e7d4 100644 --- a/dist/Math-BigInt/lib/Math/BigInt.pm +++ b/dist/Math-BigInt/lib/Math/BigInt.pm @@ -2669,20 +2669,14 @@ sub objectify { } for my $i (1 .. $count) { - my $ref = ref $a[$i]; - - # If it is an object of the right class, all is fine. - - if ($ref eq $a[0]) { - next; - } - # Don't do anything with undefs. unless (defined($a[$i])) { next; } + my $ref = ref $a[$i]; + # Perl scalars are fed to the appropriate constructor. unless ($ref) { @@ -2696,6 +2690,12 @@ sub objectify { next; } + # If it is an object of the right class, all is fine. + + if ($ref -> isa($a[0])) { + next; + } + # If we want a Math::BigInt, see if the object can become one. # Support the old misnomer as_number().
Download (untitled) / with headers
text/plain 274b
I submitted a similar patch yesterday to perlbug@perl.org with subject "[PATCH] CPAN RT 96254 and 96329: Correct the handling of subclasses." I tested the patch with Math::Currency. Without the patch, I was able to reproduce the error. With the patch, the output looked OK.
Fixed by a patch applied to blead on 24 Jun 2014.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.