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

Fixed bug "Convert hex string to decimal number #58" and made String init() 23 times faster with 60K digits. #60

Merged
merged 9 commits into from
Feb 24, 2022

Conversation

mgriebling
Copy link
Contributor

I fixed a few things including bug #58 with my merging of the different String init() functions. As a bonus, this init() is now about 23 times faster than it was and much easier to use. Overall, I was impressed by the speed of the various features with this one exception — so I had to fix it. 😉

I also fixed your magnitude function which now works correctly (at least how Apple intended).

BTW, I recommend removing the String.split since it was one major element responsible for slowing down the init().

I also noticed a bug with the random number creation where the bit sizes would be incorrect sometimes. This led me down a rabbit-hole in checking the true definition of the bitSize which is not defined how one would think. It is supposed to specify the actual size of the type on the underlying hardware — not the number of bits in a number as most people think. You can verify how Apple views this by performing the simple test: Int32(1).bitWidth = 32, not 1. Perhaps we need to define a new attribute like actualBitWidth.

I added some tags so that the Swift Package versions work correctly. The SPM does not like 'v's in front of the version numbers.

Overall, I really like the work you've done and may contribute some more bits and pieces.

Used Measure values for the sizeDescription.
Optimized the init?(String) function.
Removed the extension String and unused split().
Added a new exactBitWidth to replace the earlier bitWidth.
bitWidth is used internally so it was corrected.
Added bitWidth tests for BInt.
Fixed issue in Matrix implementation.
Fixed a duplicated init() for NumericType compliance for Double.
@twodayslate
Copy link
Collaborator

Any other tests you think should be added?

@twodayslate twodayslate requested a review from mkrd February 7, 2022 14:46
@mgriebling
Copy link
Contributor Author

Perhaps some timing tests on really long BInts from the Benchmarks.StringToBInt().
Maybe add the bug report #58 as a test case.

@twodayslate
Copy link
Collaborator

Since this PR is a direct response to #58 I think a test case should be added to address it. If you would like to add the timing test cases feel free but I don't think it is necessary but I'm also not very familiar with timing testing with Xcode.

Other than this this PR looks good to me. I'll give it a week to see if @mkrd will look at it, if not, I will merge it.

@mkrd
Copy link
Owner

mkrd commented Feb 12, 2022

@twodayslate looks good for me, too!
Thanks for your contribution @mgriebling, I’m gonna check out that 23x speedup tomorrow, that’s quite impressive!

@twodayslate
Copy link
Collaborator

@mgriebling do you want to add that test case or should I do it after merge?

@mgriebling
Copy link
Contributor Author

You can do it.

@twodayslate twodayslate merged commit ed7ce30 into mkrd:master Feb 24, 2022
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