-
Notifications
You must be signed in to change notification settings - Fork 147
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
[BigInt] Using tests from “Violet - Python VM written in Swift” #242
Comments
[3.2.2023] EDIT to main issue:
|
Things to do in
|
I wrote Violet XsProMax which is a Violet implementation with following changes:
Which gives us: struct BigInt {
struct Header {
var count: UInt32
var capacity: UInt32
}
var flags: UInt8
var buffer: ManagedBufferPointer<Header, UInt>
} I updated the performance PR with the results. |
The issue below (from @LiarPrincess) appears fixed in my latest updates (see pull request #261) // -9223372036854775808 = Int64.min, obviously '-Int64.min' overflows
let int: Int64 = -9223372036854775808
let big = BigInt(int)
XCTAssertEqual(-big, big * -1) |
All remaining issues (at least the published ones) appear to have been fixed with the pull request #262. |
This is a parent issue for merging
BigInt
tests from Violet - Python VM written in Swift. In total is is about 400 tests (about 70% of Violet test cases) split into a multiple smaller pull requests.Please note that this may seem like a lot of tests, but I still can't guarantee that everything works even after passing all of them. A few minutes ago I was debugging crash in division, just to discover that this will also fail:
Also, I have NOT looked into your implementation. I just know that you use 2 complement, but nothing more. Once we fix all of the crashes/failures I may look into your code and add some more targeted tests.
Reference implementation
BigInt
module inside Violet - Python VM written in Swift passes all of the tests (look at the swift-numerics branch).Internal Violet implementation (see documentation for more details):
Since Violet contains 2 different representations of
BigInt
(Smi
for small numbers andBigIntHeap
for the rest) in a lot of tests you will see separatetest_int_xxx
(for small integers) andtest_big_xxx
(for big integers). All of the test cases finish in ~10s (2014 rMBP, lowest spec, serial execution), so they are fast enough to keepint
tests even ifswift-numerics
implementation does not have aSmi
equivalent. This also tests if all of theBigInt
operations have the same semantic asInt
(div/shift rounding, reminder sign etc.).Platforms
All of the tests results come from 2014 rMBP -> mac 11.7 (Big Sur), Xcode 13.2.1, Intel.
Not tested:
Int.bitWidth != 64
, which is not the same as 32 bit, but whatever) - the whole Violet was designed for 64 bit (BigInt
was actually the main reason for this), so I may have unintentionally backed some assumptions here and there. Code review needed.I can confirm that Violet implementation passes tests on:
swift:latest
(5.6.0)swift:5.3.2
So, the test cases should be resistant to mac/linux/docker differences.
Missing stuff
CMakeLists.txt
, but not inBigIntTests
. I don't want to be the person to introduce it, so lets just say that I will follow theBigInt
convention of not having one.floating point tests - you already have some tests inADDEDBigIntTests.swift
.Sendable
Other engines
In some pull requests/code I will be referencing other
BigInt
implementations:WolframAlpha.com - almost all of the operations have the same semantic as Swift, except for
>>
. For example:-1932735284 >> 5
:Int64
range, so you can just-1932735284 >> 5
to test it.Both are correct, it depends on the rounding mode, but you already know this: swift-numerics/IntegerUtilities/ShiftWithRounding.swift.
Python - there is a difference in
div/mod
for negative numbers:(-5)//4
(-5)%4
Both correct:
lhs/rhs = q rem r -> lhs = q * rhs + r
.Helpers
directoryShared code for all of the test cases is inside
Helpers
directory.Since all of the pull requests contain the same
Helpers
code, it may be easier for us to first review and merge tests while ignoringHelpers
. Then, after all is merged, do any changes inHelpers
. Well… unless there is something really broken inHelpers
.License
Unless stated otherwise I am the author of this code, so feel free to slap any license you need.
Recommended merging order
In total this is around 10k hand written + 22k generated lines of code. I split them up, so that each PR contains 2-4 files (excluding
Helpers
, see above).(✅ - pass, ❌ - fail, 💀 - crash, 🐰 - to discuss)
Node
Inits and protocols
Operations
+
,-
and~
+
and-
*
,/
and%
&&
,||
and^
<<
and>>
Other
No merge
Final
I know that with so many pull requests it may be a bit hard to see the big picture, but eventually we should arrive at the state from the image below (please ignore the
Apple
andPerformance
directories,BigIntTests.swift
are the tests currently present). I think that this is quite clean (1 file for every operation), and it will be quite easy to add/modify tests.The text was updated successfully, but these errors were encountered: