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

Rt #122582 #60

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Rt #122582 #60

wants to merge 5 commits into from

Conversation

colinnewell
Copy link

This is the simplest possible fix for the test case described in RT #122582. This just ensures that the IV is copied over as well as the NV if present.

My knowledge of Perl internals isn't sufficient to really know if this is sufficient to fix the full bug described by zefram.

t/dualvar.t Outdated
@@ -131,3 +131,6 @@ SKIP: {
ok(isdual($ary[2]), 'Is a dualvar');
}

# RT #122582: dual var not copying numeric value correctly
my $a = "4611686018427387915";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is only going to pass correctly on 64bit perls surely?

@@ -1200,7 +1200,7 @@ CODE:
SvIsUV_on(TARG);
}
#endif
else {
if(SvIOK(num)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this else is going to disappear, I feel that the else on line 1197 above ought to too.

@colinnewell
Copy link
Author

Just figured I should say I will take a look at this. I've just struggled to setup a 32 bit test perl without 64 bit int support easily so I haven't actually done anything with this yet.

@colinnewell
Copy link
Author

Sorry it's taken so long, but I've rebased and updated based on your feedback. I think the Travis fail is just a glitch, if you hit rebuilt it will probably work..

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

Successfully merging this pull request may close these issues.

2 participants