Implement List::copy - #40
Merged
Merged
Conversation
List inherited Object's copy, which memcpy's the instance. For a List that duplicates head, tail and count, so both instances address the same nodes: appending to either corrupts the other's tail, and releasing either frees nodes the survivor still walks. Callers wanting a copy therefore had to build one node by node, as Array's callers have never had to. Copy as Array does, and as List's own filteredList and mappedList already do: allocate a new List and append each element in order. Elements are not retained, since append does not retain them either, and the copy does not inherit destroy, so it borrows the elements rather than owning them. That is the only safe choice for a container that does not retain, since two owning lists over the same elements would double free. The test asserts the copy is independent: same elements in the same order, distinct nodes, appending to one leaves the other's count alone, and the original survives the copy's release. It fails on the inherited implementation, on the distinct-nodes assertion.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change directly addresses the unsafe inherited copy behavior, and it includes a targeted regression test that would fail under the previous implementation.
Pull request overview
This PR fixes List’s copy behavior by overriding Object::copy (which memcpys the instance) with a node-by-node copy that produces an independent list structure, matching the semantics already used by filteredList/mappedList and aligning with how Array::copy returns an independent container.
Changes:
- Override
Object::copyforListto allocate a new list and append each element in order (no node sharing). - Add a new
copyListunit test that verifies the copy has distinct nodes, preserves element order, and remains independent after mutation and release.
File summaries
| File | Description |
|---|---|
| Sources/Objectively/List.c | Implements List-specific copy to avoid shared nodes and corruption from the inherited memcpy copy. |
| Tests/Objectively/List.c | Adds a regression test asserting copied lists are structurally independent and safe to mutate/release. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
List inherited Object's copy, which memcpy's the instance. For a List that duplicates head, tail and count, so both instances address the same nodes: appending to either corrupts the other's tail, and releasing either frees nodes the survivor still walks. Callers wanting a copy therefore had to build one node by node, as Array's callers have never had to.
Copy as Array does, and as List's own filteredList and mappedList already do: allocate a new List and append each element in order. Elements are not retained, since append does not retain them either, and the copy does not inherit destroy, so it borrows the elements rather than owning them. That is the only safe choice for a container that does not retain, since two owning lists over the same elements would double free.
The test asserts the copy is independent: same elements in the same order, distinct nodes, appending to one leaves the other's count alone, and the original survives the copy's release. It fails on the inherited implementation, on the distinct-nodes assertion.