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

[BigInt tests] ✅ Copy on write #249

Open
wants to merge 2 commits into
base: biginteger
Choose a base branch
from

Conversation

LiarPrincess
Copy link

Please read the #242 Using tests from “Violet - Python VM written in Swift” before.


All pass.

🐰 Discussion

Those tests are a bit difficult to grasp. Especially because they start with unary operations. I think it would be better if you started with binary operations:

func test_add_toCopy_doesNotModifyOriginal() {
  // int + int
  var value = BigInt(Int.max)
  var copy = value
  _ = copy + self.int
  XCTAssertEqual(value, BigInt(Int.max))
}

func test_addEqual_toCopy_doesNotModifyOriginal() {
  // int + int
  var value = BigInt(Int.max)
  var copy = value
  copy += self.int
  XCTAssertEqual(value, BigInt(Int.max))
}

The general idea is that the original value should not be modified when we perform operation on copy. It will not test if the copy was created before (var copy = value) or during the write (actual COW), but it will test that the copy was made at some point.

The general idea is that BigInt will probably allocate on the heap. What happens when we modify the heap value? All of them change? Or maybe only the BigInt that was used in operation? It is easy to break the invariant if you implement COW by hand.

Alternatively you could make BigInt non-copyable (move-only). Then you chose:

  1. copy heap on BigInt copy (no COW)
  1. reference count, so that BigInt is a struct with explicit copy/retain (COW, acting like a smart pointer, RAII etc.)

Obviously move-only is extremely un-ergonomic, so…

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.

1 participant