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. #59

Closed
wants to merge 8 commits into from

Conversation

mgriebling
Copy link
Contributor

@mgriebling mgriebling commented Feb 6, 2022

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.

@@ -20,6 +20,12 @@ class BIntTests: XCTestCase {
// Put teardown code here. This method is called after the invocation of each test method in the class.
super.tearDown()
}

// func testSizeString()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove or actually implement

Copy link
Contributor Author

@mgriebling mgriebling Feb 7, 2022

Choose a reason for hiding this comment

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

Removed it. A remnant of a test of mine.

@@ -17,6 +17,7 @@ protocol NumericType:

extension Double: NumericType
{
// FIXME: Compiler warns about function call causing infinite recursion - MG
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you planning on actually fixing these warnings in this PR?

Copy link
Contributor Author

@mgriebling mgriebling Feb 7, 2022

Choose a reason for hiding this comment

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

Fixed it. It turns out this was implemented elsewhere so I just deleted the code.

return String(self[start..<end])
}
}
// FIXME: The *split* causes a massive overhead due to *self* calls. Please remove.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If split isn't used anymore then please remove this extension entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will do.

@@ -1257,6 +1280,11 @@ fileprivate extension Array where Element == Limb

/// Returns the number of bits that contribute to the represented number, ignoring all
/// leading zeros.

// FIXME: - Not the correct definition of *bitWidth* according to Apple - MG 6 Feb 2022
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a unit test to match the correct definition

Copy link
Contributor Author

@mgriebling mgriebling Feb 7, 2022

Choose a reason for hiding this comment

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

Can't do, the Limb bitWidth is fileprivate. Instead, I changed the name to exactBitWidth and updated the places where it is used. The implementation is unchanged.

@mgriebling mgriebling closed this Feb 7, 2022
@mgriebling
Copy link
Contributor Author

I'll submit an updated pull request shortly.

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