Skip to content

fix: preserve matrix object identity#130

Merged
luisleo526 merged 1 commit into
mainfrom
codex/matrix-object-identity-post129-20260722
Jul 22, 2026
Merged

fix: preserve matrix object identity#130
luisleo526 merged 1 commit into
mainfrom
codex/matrix-object-identity-post129-20260722

Conversation

@luisleo526

Copy link
Copy Markdown
Collaborator

Summary

  • give generic, bool, and numeric Pine matrices shared backing identity so ordinary assignment aliases the same matrix object
  • retain an explicit detached copy() operation and preserve null/valid-empty behavior
  • integrate matrix mutation checkpoints with historical rollback without weakening Release assertions

Root cause

The runtime wrappers owned matrix state by value. C++ assignment therefore detached the destination even though Pine matrix IDs are reference objects. That value behavior could be hidden by codegen-side UDT copies, typed method dispatch, and checkpoint restoration, so it was validated as independent factor D in the full A/B/C/D matrix.

Scope boundary

This PR changes only matrix object identity and its focused tests. It does not add direct-array UDT field support and does not modify strategy sources, TradingView tapes, settings, graders, or tier data. None of the seven changed paths has a Git LFS attribute.

Validation

  • clean Release build: passed
  • full CTest: 117/117 passed
  • focused matrix identity target: passed
  • exhaustive cross-repository N=4 gate: 16/16 cells, 80/80 probe executions, adjudicator GO
  • git diff --check: clean

Companion milestones are merged: codegen #100 and exhaustive harness #119. The engine base already contains snapshot bridge #128 and Release assertion #129.

Copilot AI review requested due to automatic review settings July 22, 2026 10:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates Pine matrix wrappers to preserve PineScript-style matrix object identity (reference IDs) across ordinary C++ copies/assignments, while keeping copy() as the explicit detach operation. It also aligns snapshot/restore and Series tests with the new aliasing semantics.

Changes:

  • Rework PineMatrix, PineGenericMatrix<T>, and PineGenericMatrix<bool> to share backing storage via std::shared_ptr, so copy/assignment aliases the same matrix ID.
  • Update snapshot/restore to retain and mutate the original identity in-place (so existing aliases observe rollbacks), with explicit invalid-snapshot errors.
  • Add/adjust focused tests covering identity, moves, snapshot behavior, and Series aliasing expectations.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/test_matrix_snapshot_compat.cpp Updates snapshot compatibility assertions to reflect aliasing-on-assignment semantics.
tests/test_matrix_identity.cpp New targeted test binary validating aliasing, detach via copy(), move semantics, and snapshot identity behavior.
tests/test_generic_matrix_series.cpp Updates Series test to assert that history slots preserve matrix ID aliasing rather than value-detaching.
tests/CMakeLists.txt Registers the new test_matrix_identity executable in the test suite.
src/matrix.cpp Implements storage-backed identity, updated snapshot/restore, and routes operations through storage accessors.
include/pineforge/matrix.hpp Switches PineMatrix to shared storage, defines identity-preserving move semantics, and snapshot identity retention.
include/pineforge/generic_matrix.hpp Applies the same shared-identity model to generic and bool matrices, including snapshot identity retention and restore behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@luisleo526
luisleo526 merged commit 7238676 into main Jul 22, 2026
8 checks passed
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