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

Report information
The Basics
Id:
96329
Status:
resolved
Priority:
Low/Low
Queue:

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

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



Subject: () breaks non-foreign classes
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.
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>
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
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.
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
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().
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 runs on Request Tracker, is sponsored by The Perl Foundation, and maintained by Best Practical Solutions.

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