Skip to content

Implement List::copy - #40

Merged
jdolan merged 1 commit into
mainfrom
feature/list-copy
Aug 21, 2026
Merged

Implement List::copy#40
jdolan merged 1 commit into
mainfrom
feature/list-copy

Conversation

@jdolan

@jdolan jdolan commented Aug 21, 2026

Copy link
Copy Markdown
Owner

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.

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.
Copilot AI lite review requested due to automatic review settings August 21, 2026 20:22
@jdolan
jdolan merged commit 9b9c272 into main Aug 21, 2026
3 checks passed
@jdolan
jdolan deleted the feature/list-copy branch August 21, 2026 20:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 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::copy for List to allocate a new list and append each element in order (no node sharing).
  • Add a new copyList unit 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.

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