Skip to content

Fix #164: rebind spans after copy/move/assignment#400

Merged
jurasic-pf merged 4 commits intoproximafusion:mainfrom
jungdaesuh:fix-issue-164-long-term
Feb 11, 2026
Merged

Fix #164: rebind spans after copy/move/assignment#400
jurasic-pf merged 4 commits intoproximafusion:mainfrom
jungdaesuh:fix-issue-164-long-term

Conversation

@jungdaesuh
Copy link
Contributor

@jungdaesuh jungdaesuh commented Jan 10, 2026

Summary

Fixes Issue #164: FourierGeometry/FourierVelocity/FourierForces store std::span views into FourierCoeffs-owned vectors. The compiler-generated copy/move semantics shallow-copy those spans, so after a copy the spans can still point at the source object's buffers.

This PR implements copy/move constructors and copy/move assignment operators for:

  • FourierGeometry
  • FourierVelocity
  • FourierForces

These operations rebind the span members to the destination object's own storage after copy/move/assignment.

It also adds copy/move assignment operators to FourierCoeffs (const config members delete the implicit operator=).

Testing

  • bazel test --test_output=errors --nocache_test_results //vmecpp_large_cpp_tests/vmec/fourier_geometry:fourier_geometry_test
  • bazel test --test_output=errors --nocache_test_results //vmecpp/...

Limitations / follow-up

  • FourierCoeffs keeps config as const refs/ints (Sizes/RadialPartitioning/ns*), so FourierCoeffs::operator= copies/moves only coefficient vectors and does not update config. Assignment is intended only between same-config objects; cross-config assignment is unsupported/undefined.
  • Follow-up idea (API-breaking for C++): rule-of-zero value semantics by storing needed config scalars by value and replacing stored span members with span-returning accessors.

@jungdaesuh jungdaesuh force-pushed the fix-issue-164-long-term branch from cf74da6 to b25868a Compare January 10, 2026 04:37
@jungdaesuh jungdaesuh changed the title Fix FourierCoeffs copy/move span rebinding Fix Fourier copy/move span rebinding and netCDF bytes handling Jan 10, 2026
@jurasic-pf
Copy link
Collaborator

Thanks for looking into this issue.
Could you demonstrate that the copy+move semantics are now working as expected in a test case please? It looks good on first review though, please just split the changes related to the copy move issue and type checking improvements into separate PRs.

@jungdaesuh
Copy link
Contributor Author

@jurasic-pf Thanks for the review. I will reply to specific comments once I sorted things out. It's like solving one issue keeps leading to another issue.

@jungdaesuh jungdaesuh force-pushed the fix-issue-164-long-term branch from b25868a to 56ec23c Compare February 5, 2026 05:24
@jungdaesuh jungdaesuh changed the title Fix Fourier copy/move span rebinding and netCDF bytes handling Fix #164: rebind spans after copy/move/assignment Feb 5, 2026
@jungdaesuh jungdaesuh force-pushed the fix-issue-164-long-term branch from 56ec23c to 1765fcc Compare February 5, 2026 05:31
Copy link
Collaborator

@jurasic-pf jurasic-pf left a comment

Choose a reason for hiding this comment

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

Great! Thanks for taking the time, I just added a test to confirm behavior is as expected.

@jurasic-pf jurasic-pf merged commit 80f1ade into proximafusion:main Feb 11, 2026
23 checks passed
@jungdaesuh
Copy link
Contributor Author

Hi @jurasic-pf, sorry for the slow progress. I picked up #164 to make contributions to this repo and went with the minimal fix (rebind spans after copy/move/assignment). In hindsight I should have asked direction first.

If useful, I can follow up with a separate PR to:

  1. add a focused in-repo copy/move semantics test, (I have it on my local, and was about to push it)
  2. Or, pivot to accessor-based views (rule-of-zero design)
  • (Initially considered 'delete copy/move for now' too)

Which direction do you prefer? I am happy to pick up other issues if you guys need extra hands. I would like to contribute.

@jungdaesuh
Copy link
Contributor Author

@jurasic-pf I see that you've also added the test. Thank you!

@jungdaesuh
Copy link
Contributor Author

@jurasic-pf I can't work on this full time, but would like to make contributions to this repo. I'll try to be more consistent.

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.

Copy and move semantics of FourierCoeffs and deriving classes are broken

2 participants