Skip to content

Conversation

@SineSwiper
Copy link

Still having any problems testing this branch or does everything look good?

kenahoo and others added 3 commits June 15, 2012 22:58
 - Complete support for large numbers in all directions, and
   for large floats.
 - Ability to specify a different negative sign or radix point,
   if - or . will be used as a digit.
 - Short-circuiting to the more efficient sprintf/oct functions
   for bin/hex/oct conversions.
 - Ditching Horner<92>s algorithm in favor of exponent-based one that
   is more accurate for fractionals.
 - Module will die if an invalid character is found.
 - Changed tests to use new_ok if possible.
 - Added Unicode tests to make sure they continue to work.
Add to .gitignore
Removed blank t/02_digit_dash.t (was actually a rename operation)
Small corrections on 06-unicode.t test
Copy link
Author

Choose a reason for hiding this comment

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

Hmmm...if I'm re-reviewing this code right, the possibility exists that this could break for big numbers, since it's trying to do math on a string. This should be:

return accurate_return( Math::BigFloat->new( $self->from_base($str) )->bneg() )
  if $nc && $str =~ s/^\Q$nc\E//; # Handle negative numbers

Where accurate_return() is that code piece on the bottom of from_base. I'll add a commit with that and a test for large negatives. Also, it looks like to_base negatives needs a quick big number check to make sure it can do -1*$num.

@SineSwiper
Copy link
Author

My version of bbyrd has the latest commit.

@SineSwiper
Copy link
Author

FYI, you might want to merge in this last change:

SineSwiper@6546f19

I figured out the nuances of Big*'s upgrade routine, and it should upgrade properly in these cases.

@kenahoo
Copy link
Owner

kenahoo commented Aug 8, 2012

Thanks. I've merged it in locally (got a small conflict because of the last class-name patch), now I need to test, which means installing Dist::Zilla and all its dependencies, so 3.5 billion years later I should be good to go, assuming things test out okay.

@kenahoo
Copy link
Owner

kenahoo commented Aug 8, 2012

I'm still getting a lot of failures, looks about the same as before. Here's the first failure:


t/01_basic_tests.t ..... 1/15
#   Failed test 'negative hex (-11) into decimal (-17)'
#   at t/01_basic_tests.t line 47.
#          got: '-17e+0'
#     expected: '-17'

I can commit this (to bbyrd branch) if you'd like?

@kenahoo
Copy link
Owner

kenahoo commented Aug 8, 2012

Here's a little test that shows the root of the problem.

% perl -MMath::BigFloat -le 'print Math::BigFloat->new(11)->numify'
11e+0

@SineSwiper
Copy link
Author

Bah, stupid BigMath bugs:

loubbyrd@noclab01:~$ perl -MMath::BigFloat -le "print Math::BigFloat->new(11)->numify"
11e+0
loubbyrd@noclab01:~$ perl -MMath::BigFloat -E 'say $Math::BigFloat::VERSION'
1.60

Z:\code\_others>perl -MMath::BigFloat -le "print Math::BigFloat->new(11)->numify"
11

Z:\code\_others>perl -MMath::BigFloat -E "say $Math::BigFloat::VERSION"
1.997

I was thinking of fixing it here, but you know what... 1.60 was made in 2002. Making a dep of a BigMath version that was updated sometime after the Nixon administration might be a better idea :)

Thoughts?

@kenahoo
Copy link
Owner

kenahoo commented Aug 13, 2012

I think it's fine to depend on a later version where it's fixed, as long as that's available without upgrading perl. And that seems to be the case, so I'd say go for it.

@SineSwiper
Copy link
Author

Interesting. The problem was only fixed in 1.997, so we'll need to make the dep be "Math::BigFloat >= 1.997". Version 1.993 still has the issue.

@SineSwiper
Copy link
Author

Okay, I got back to this issue, rebased it, and fixed the problems in such a way that it would work on older versions of BigFloat. The last commit is SineSwiper/perl-math-basecalc@579656e676a, and that forked branch has the rebasing.

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.

3 participants