Skip to content

Fix complex matrix assignment ownership#4914

Open
fallintoplace wants to merge 2 commits into
NVIDIA:mainfrom
fallintoplace:fix/complex-matrix-assignment
Open

Fix complex matrix assignment ownership#4914
fallintoplace wants to merge 2 commits into
NVIDIA:mainfrom
fallintoplace:fix/complex-matrix-assignment

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • make copy assignment release the previous allocation through copy-and-swap
  • make move assignment safely transfer ownership through swap, including self-move
  • mark move construction and assignment noexcept
  • cover repeated assignments across empty and differently sized matrices, self-copy, and self-move

Both assignment operators previously overwrote an owning pointer without releasing its allocation. Aliasing the source made the behavior worse: self-copy discarded the original values, while self-move nulled its own pointer.

Testing

  • pre-commit run --files runtime/cudaq/operators/matrix.h unittests/utils/Matrix.cpp
  • macOS Apple Clang ASan+UBSan harness: 10,000 repeated copy/move assignments and self-assignment checks
  • macOS leaks: 0 leaks

@copy-pr-bot

copy-pr-bot Bot commented Jul 10, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@1tnguyen

1tnguyen commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

/ok to test fd0616d

Command Bot: Processing...

Comment thread runtime/cudaq/operators/matrix.h Outdated

@1tnguyen 1tnguyen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall, LGTM 👍 Thanks for the contribution @fallintoplace

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

CI Summary (push) — ✅ passed

Run #29260722704 · ✅ 6 · ⏩ 7 · ❌ 0 · ⛔ 0

Top-level jobs (13)
Job Result
binaries ⏩ skipped
build_and_test ✅ success
config_devdeps ✅ success
config_source_build ⏩ skipped
config_wheeldeps ✅ success
devdeps ✅ success
docker_image ⏩ skipped
gen_code_coverage ⏩ skipped
metadata ✅ success
python_metapackages ⏩ skipped
python_wheels ⏩ skipped
source_build ⏩ skipped
wheeldeps ✅ success
⏩ Skipped jobs (7) — intentionally skipped on PR builds; run on merge_group / workflow_dispatch
Job
binaries
config_source_build
docker_image
gen_code_coverage
python_metapackages
python_wheels
source_build
All sub-jobs (42) — every matrix leg, with links
Job Status Link
Build and test (amd64, gcc12, openmpi) / Dev environment (Debug) ✅ success view
Build and test (amd64, gcc12, openmpi) / Dev environment (Python) ✅ success view
Build and test (amd64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (amd64, llvm, openmpi) / Dev environment (Python) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Python) ✅ success view
CI Summary ❔ in_progress view
Configure build (devdeps) ✅ success view
Configure build (source_build) ⏩ skipped view
Configure build (wheeldeps) ✅ success view
Create CUDA Quantum installer ⏩ skipped view
Create Docker images ⏩ skipped view
Create Python metapackages ⏩ skipped view
Create Python wheels ⏩ skipped view
Gen code coverage ⏩ skipped view
Load dependencies (amd64, gcc12) / Caching ✅ success view
Load dependencies (amd64, gcc12) / Finalize ✅ success view
Load dependencies (amd64, gcc12) / Metadata ✅ success view
Load dependencies (amd64, llvm) / Caching ✅ success view
Load dependencies (amd64, llvm) / Finalize ✅ success view
Load dependencies (amd64, llvm) / Metadata ✅ success view
Load dependencies (arm64, gcc12) / Caching ✅ success view
Load dependencies (arm64, gcc12) / Finalize ✅ success view
Load dependencies (arm64, gcc12) / Metadata ✅ success view
Load dependencies (arm64, llvm) / Caching ✅ success view
Load dependencies (arm64, llvm) / Finalize ✅ success view
Load dependencies (arm64, llvm) / Metadata ✅ success view
Load source build cache ⏩ skipped view
Load wheel dependencies (amd64, 12.6) / Caching ✅ success view
Load wheel dependencies (amd64, 12.6) / Finalize ✅ success view
Load wheel dependencies (amd64, 12.6) / Metadata ✅ success view
Load wheel dependencies (amd64, 13.0) / Caching ✅ success view
Load wheel dependencies (amd64, 13.0) / Finalize ✅ success view
Load wheel dependencies (amd64, 13.0) / Metadata ✅ success view
Load wheel dependencies (arm64, 12.6) / Caching ✅ success view
Load wheel dependencies (arm64, 12.6) / Finalize ✅ success view
Load wheel dependencies (arm64, 12.6) / Metadata ✅ success view
Load wheel dependencies (arm64, 13.0) / Caching ✅ success view
Load wheel dependencies (arm64, 13.0) / Finalize ✅ success view
Load wheel dependencies (arm64, 13.0) / Metadata ✅ success view
Prepare cache clean-up ❔ in_progress view
Retrieve PR info ✅ success view
✅ Required checks (6/6) — declared in .github/required-checks.yml for push
Required check Status Link
Build and test (amd64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (amd64, llvm, openmpi) / Dev environment (Python) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Python) ✅ success view
Build and test (amd64, gcc12, openmpi) / Dev environment (Debug) ✅ success view
Build and test (amd64, gcc12, openmpi) / Dev environment (Python) ✅ success view

@fallintoplace
fallintoplace force-pushed the fix/complex-matrix-assignment branch from 545b1a3 to fd7ad5c Compare July 13, 2026 13:19
@sacpis

sacpis commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

/ok to test 889822d

Command Bot: Processing...

@sacpis
sacpis enabled auto-merge July 13, 2026 15:04
Comment thread runtime/cudaq/operators/matrix.h Outdated
Comment on lines +237 to +240
using std::swap;
swap(dimensions, other.dimensions);
swap(data, other.data);
swap(internal_order, other.internal_order);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
using std::swap;
swap(dimensions, other.dimensions);
swap(data, other.data);
swap(internal_order, other.internal_order);
std::swap(dimensions, other.dimensions);
std::swap(data, other.data);
std::swap(internal_order, other.internal_order);

@sacpis
sacpis disabled auto-merge July 13, 2026 15:35
Comment thread runtime/cudaq/operators/matrix.h Outdated
Comment on lines +91 to +92
other.data = nullptr;
other.dimensions = {};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's weird to me that this doesn't use swap but copy assignment builds a temporary so that it can use swap instead of a simple deallocation.

Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
@fallintoplace
fallintoplace force-pushed the fix/complex-matrix-assignment branch from 889822d to 1eaa5f1 Compare July 14, 2026 00:42
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.

4 participants