search

ionspin / kotlin-multiplatform-bignum

A Kotlin multiplatform library for arbitrary precision arithmetics
Apache License 2.0
366 stars 42 forks source link

Apply skolsons solution from #131 double to float operations as well.… #135

Closed ionspin closed 3 years ago

ionspin commented 4 years ago

… This hopefully resolves #134

ionspin commented 4 years ago

I kinda blindly applied the solution for 131 here, it looks like it should work properly, but I would need @skolson to verify, and I'll try to look a bit more into it over weekend.

ionspin commented 4 years ago

Need to return the check if number can be expressed in IEEE 754

ionspin commented 3 years ago

So at this point it should be working correctly for normal numbers, but there needs to be a check for subnormal numbers.

ionspin commented 3 years ago

@skolson If you have time to give this a glance, just to check if I missed anything obvious, otherwise I'd like to merge this to master this week. A fix for string parsing sneaked into this commit in the end, because exponent wasn't properly calculated when parsing mixed scientific notation and expanded such as 0.0000123E20 and 123E20 instead of pure scientific like 1.23E15 and 1.23E22 respectively.

skolson commented 3 years ago

I have one more commit with changes to BigDecimal.kt that removes the string massaging in fromFloat and fromDouble targeting trailing insignificant zeroes, and moves the trailing zero removal to parseStringWithMode. I'll get that pushed up in a bit...

skolson commented 3 years ago

My git skills suck :-) Can you look at my fork and the two commits (the one mentioned above, plus some additional tests) that I want to add to this pull request: skolson fork? If the changes in the comparison looks ok (tests pass), can you walk me through the git commands to add this to this pull request? I'm doing something wrong :-) On my side it looks like these should merge fine, but I would appreciate help. Thanks!

ionspin commented 3 years ago

hey @skolson, it looks like it would merge fine because it's comparing to master branch, and not this pull request branch. But I can also tell you immediately, that I changed almost all of the lines your commits are referring to so I don't think it would be useful to merge them at this point.

Also I don't think you can update a pull request if you are not added as a collaborator on a project, but I'm not sure 100% sure about that.

I'll go ahead and merge this pull request into master, and then we can look at your pull request, but again, you'll see that I changed a LOT there.

skolson commented 3 years ago

Cool! Once the new stuff is in the snapshot I'll try it out. Ans I'll get my fork and local source synced up. Thanks!