refactor(c++): ♻️ adhere to rule of zero - #239
Draft
robertodr wants to merge 5 commits into
Draft
Conversation
`MonomialPropagator` declared a destructor and a copy constructor, and deleted
copy assignment, for one reason: `partition_group_` is a `unique_ptr`, so the
implicit copy would have been deleted. The cost was a copy constructor that
named all 17 members by hand -- a member added later would have been
default-initialized in every copy, silently -- and suppressed move operations,
so every "move" of a propagator bound to the copy constructor and deep-copied
the whole operator store. `MPOperator` carried the same pair of problems for its
`store`, with 10 members named by hand.
Add `value_ptr<T>` (`cpp/monoprop/ValuePtr.h`): a `unique_ptr` that copies its
pointee, preferring `T::clone()` when the type has one and falling back to `T`'s
copy constructor. Copy assignment clones before it releases, so `T` need not be
assignable and self-assignment needs no guard. Unlike `unique_ptr`, `const`
propagates to the pointee -- the pointee is a value member here.
Holding `partition_group_` and `MPOperator::store` in it lets both classes
declare no special member at all. The compiler now supplies:
- copy, as deep as before: the store clones via `OperatorIndex::clone()`, the
partition group clones (fresh transport, fresh masters, rebound comms), and
the immutable layer cores stay shared through their `shared_ptr`s;
- a real move, which steals the store instead of deep-copying it, and is
still `noexcept` -- what the explicit `noexcept = default` pair on
`MPOperator` used to assert;
- copy and move assignment, which were unavailable before.
Nothing held a `MonomialPropagator` or `MPOperator` by value in a container, so
no existing path changes behaviour; results are bit-identical over the same 95
expectation-value and gradient fingerprints used for the picture refactor.
Adds `value_ptr_tests.cpp` for the two copy routes, self-assignment, the move
path, the empty case and const propagation, plus propagator tests for the two
newly available operations: a deep copy-assignment, and a move that carries the
store address over.
Assisted-by: ClaudeCode:claude-opus-5
robertodr
requested review from
diagonal-hamiltonian,
fpietra and
ludmilaasb
as code owners
August 17, 2026 11:55
github-actions
Bot
added
documentation
robertodr
requested review from
Panadestein
and
a balanced review from Copilot
August 17, 2026 11:55
|
Docs preview: https://pr-239.monoprop-docs.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #239 +/- ##
=======================================
Coverage 97.70% 97.70%
=======================================
Files 14 14
Lines 742 742
Branches 98 98
=======================================
Hits 725 725
Misses 12 12
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. |
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces value_ptr to provide deep-copy value semantics and refactors propagator storage toward the Rule of Zero.
Changes:
- Adds and tests
value_ptr. - Refactors
MPOperatorandMonomialPropagatorownership. - Expands copy/move tests and documentation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
AGENTS.md |
Documents Rule-of-Zero guidance. |
cspell.json |
Adds relevant vocabulary. |
cpp/monoprop/CMakeLists.txt |
Installs the new header. |
cpp/monoprop/ValuePtr.h |
Implements value_ptr. |
cpp/monoprop/detail/operator/MPOperator.h |
Uses value-semantic storage. |
cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl |
Removes manual special members. |
cpp/include/monoprop/MonomialPropagator.h |
Changes ownership and public polymorphism. |
cpp/tests/value_ptr_tests.cpp |
Tests pointer semantics. |
cpp/tests/simulator_copy_tests.cpp |
Tests propagator copying and moving. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+71
to
+78
| if constexpr (requires { | ||
| { src->clone() } -> std::convertible_to<std::unique_ptr<T>>; | ||
| }) { | ||
| return src->clone(); | ||
| } | ||
| else { | ||
| return std::make_unique<T>(*src); | ||
| } |
Comment on lines
+37
to
+38
| static_assert(std::is_copy_assignable_v<MonomialPropagator<8>>, "simulator must be copy-assignable"); | ||
| static_assert(std::is_move_assignable_v<MonomialPropagator<8>>, "simulator must be move-assignable"); |
robertodr
changed the title
|
robertodr
marked this pull request as draft
August 17, 2026 18:12
but of course there are memory footguns when going for rule-of-zero, since {copy,move} {CTOR,assignment} are back in the game automatically.
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.
Summary
Ensure that we adhere to the rule of zero which reduces the amount of boilerplate code, see also here. In C++26 there is
std::indirect<T>for exactly this purpose, here the bot re-created it asvalue_ptr<T>.🤖 AI text below 🤖
This pull request introduces a new
value_ptrsmart pointer type to enable deep-copyable, value-semantics heap members in core classes, and refactors the codebase to use it. This allows classes likeMonomialPropagatorandMPOperatorto follow the Rule of Zero, eliminating the need for hand-written copy/move constructors and destructors. The update also brings improved test coverage for copy/move semantics and the new pointer type.The most important changes are:
Core infrastructure: value_ptr
value_ptrsmart pointer (cpp/monoprop/ValuePtr.h), which enables exclusive-ownership heap members with value semantics, supporting bothT::clone()and copy-construction for deep copies. Includes amake_valuehelper.Refactoring to Rule of Zero
MonomialPropagatorandMPOperatorto usevalue_ptrinstead ofunique_ptrfor heap-owned members (partition_group_,store), removing all explicit copy/move constructors, destructors, and assignment operators, so the compiler-generated ones are used (Rule of Zero). [1] [2] [3] [4] [5] [6] [7]Testing and validation
value_ptrcovering both clone-based and copy-constructor-based deep copying, assignment, move semantics, const propagation, and empty-pointer behavior (cpp/tests/value_ptr_tests.cpp).cpp/tests/simulator_copy_tests.cpp). [1] [2]Documentation and build
AGENTS.md) to require following the Rule of Zero for classes, usingvalue_ptrfor heap-owned members, and avoiding hand-written copy/move/destructor code.ValuePtr.hwhere needed. [1] [2] [3]Minor code and comment updates
Checklist
docs/,CONTRIBUTING.md) if neededCHANGELOG/ release notes updated if applicableAI/LLM disclosure
Important
By opening this PR I confirm that I have read CONTRIBUTING.md and I agree to the terms of the Contributor License Agreement.
Warning
If you're contributing on behalf of your employer, contact cla@algorithmiq.fi to arrange a Corporate CLA.