From 8cc401a99a7d0f2d8a2abb0399babddc9ba5c64c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Mon, 17 Aug 2026 08:37:59 +0000 Subject: [PATCH 1/4] refactor(c++): :broom: put `MonomialPropagator` on the Rule of Zero `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` (`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 --- AGENTS.md | 4 + cpp/include/monoprop/MonomialPropagator.h | 14 +- cpp/monoprop/CMakeLists.txt | 1 + cpp/monoprop/ValuePtr.h | 90 +++++++++++++ .../MonomialPropagator.inl | 31 +---- cpp/monoprop/detail/operator/MPOperator.h | 25 +--- cpp/tests/simulator_copy_tests.cpp | 51 +++++++- cpp/tests/value_ptr_tests.cpp | 120 ++++++++++++++++++ cspell.json | 2 + 9 files changed, 279 insertions(+), 59 deletions(-) create mode 100644 cpp/monoprop/ValuePtr.h create mode 100644 cpp/tests/value_ptr_tests.cpp diff --git a/AGENTS.md b/AGENTS.md index a932c4ab..a424c394 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,10 @@ monoprop is a high-performance C++/Python hybrid library implementing Majorana a - Python docstrings use Google style, and are rendered into the docs site by `just gen-api` — keep them accurate. - In prose docs (`docs/content/docs/**.mdx`) and Python docstrings, link to API symbols with the mkdocstrings-style `[Symbol][]` reference (or `[Display][fully.qualified.path]`) — never hard-code `/api/...` URLs. Do not backtick the name in the `[Symbol][]` form. See `docs/content/docs/documenting.mdx`. +- Keep classes on the Rule of Zero: declare no destructor, copy or move member. A member that needs a + deep copy because it is heap-owned goes in `value_ptr` (`cpp/monoprop/ValuePtr.h`), which clones its + pointee — through `T::clone()` when the type has one — rather than in a `unique_ptr` plus a hand-written + copy constructor that has to name every other member and silently drops any member added later. - C++ comments: bare `//` for one-line comments, `/* */` for block comments. - Add Doxygen Qt-style documentation on all declarations in header files. Put documentation after members in enums, structs, classes. diff --git a/cpp/include/monoprop/MonomialPropagator.h b/cpp/include/monoprop/MonomialPropagator.h index f80f3e87..b740e455 100644 --- a/cpp/include/monoprop/MonomialPropagator.h +++ b/cpp/include/monoprop/MonomialPropagator.h @@ -39,6 +39,7 @@ #include "monoprop/TypeAliases.h" #include "monoprop/Utilities.h" #include "monoprop/Validation.h" +#include "monoprop/ValuePtr.h" #include "monoprop/algebra/PauliAlgebra.h" #include "monoprop/detail/evolution/CosineRecompute.h" #include "monoprop/detail/mpi/MPICompat.h" @@ -54,8 +55,7 @@ class PartitionGroup; } // namespace detail /// A propagator setting is out of range, or inconsistent with another setting. -// Covers a crossed atol pair and a logical width outside [1, NumModes]; also thrown from -// MonomialPropagatorImpl.h +// Covers a crossed atol pair and a logical width outside [1, NumModes]. class PropagatorConfigError : public std::runtime_error { public: using std::runtime_error::runtime_error; @@ -83,14 +83,6 @@ class MonomialPropagator { Basis basis = Basis::Majorana, size_t partitions = 0); - /// Out-of-line because partition_group_ is a unique_ptr to an incomplete type here. - virtual ~MonomialPropagator(); - - /// Deep copy: clones the operator store, shares the immutable graph cores, and clones the whole - /// partition group on a facade. The virtual destructor suppresses implicit moves, so a "move" deep-copies. - MonomialPropagator(const MonomialPropagator &other); - auto operator=(const MonomialPropagator &) -> MonomialPropagator & = delete; - static constexpr auto num_modes{NumModes}; static constexpr auto storage_num_modes{NumModes}; @@ -322,7 +314,7 @@ class MonomialPropagator { // Intra-process partition runtime. Null ⇒ ordinary single-partition propagator; non-null ⇒ a partition facade // whose own mp_op_/graph_ are unused and every method fans out to the S partition propagators. - std::unique_ptr> partition_group_; + value_ptr> partition_group_; // PartitionGroup rebinds a cloned partition's comm_ to its own transport during a deep copy. friend class detail::partition::PartitionGroup; diff --git a/cpp/monoprop/CMakeLists.txt b/cpp/monoprop/CMakeLists.txt index b18c8b33..3d8b93da 100644 --- a/cpp/monoprop/CMakeLists.txt +++ b/cpp/monoprop/CMakeLists.txt @@ -91,6 +91,7 @@ target_sources( "${PROJECT_SOURCE_DIR}/cpp/monoprop/TypeAliases.h" "${PROJECT_SOURCE_DIR}/cpp/monoprop/Utilities.h" "${PROJECT_SOURCE_DIR}/cpp/monoprop/Validation.h" + "${PROJECT_SOURCE_DIR}/cpp/monoprop/ValuePtr.h" ) target_compile_definitions( diff --git a/cpp/monoprop/ValuePtr.h b/cpp/monoprop/ValuePtr.h new file mode 100644 index 00000000..570867ce --- /dev/null +++ b/cpp/monoprop/ValuePtr.h @@ -0,0 +1,90 @@ +// Copyright 2026 Algorithmiq +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include +#include +#include + +namespace monoprop { + +/// A `unique_ptr` that copies its pointee instead of refusing to copy. +// Exclusive ownership with value semantics. It exists so that a class holding a heap member -- because +// the member is non-copyable, non-movable, or incomplete at that point -- needs no hand-written copy +// constructor or destructor, and so keeps its implicit move operations: the Rule of Zero. A hand-written +// copy constructor has to name every other member too, and silently default-initializes any member added +// later. +// +// Copying prefers `T::clone()` when the pointee declares one, and falls back to `T`'s copy constructor. +// A type that owns internal indices usually offers clone() precisely because it forbids copy construction. +// +// `T` may be incomplete where `value_ptr` is named: as with `unique_ptr`, each member body that needs +// `T` complete is instantiated only at its own point of use. +template +class value_ptr { +public: + value_ptr() = default; + + explicit value_ptr(std::unique_ptr owned) noexcept : ptr_(std::move(owned)) {} + + value_ptr(const value_ptr &other) : ptr_(clone_of_(other.ptr_)) {} + + auto operator=(const value_ptr &other) -> value_ptr & { + // Clone and replace, never T's assignment: T need not be assignable, and taking the clone before + // the old pointee is released makes self-assignment safe without a guard. + ptr_ = clone_of_(other.ptr_); + return *this; + } + + value_ptr(value_ptr &&) noexcept = default; + auto operator=(value_ptr &&) noexcept -> value_ptr & = default; + ~value_ptr() = default; + + // const propagates to the pointee, unlike unique_ptr's: the pointee is a value member here, not a + // pointer the object happens to hold. + auto operator*() -> T & { return *ptr_; } + auto operator*() const -> const T & { return *ptr_; } + auto operator->() -> T * { return ptr_.get(); } + auto operator->() const -> const T * { return ptr_.get(); } + auto get() -> T * { return ptr_.get(); } + auto get() const -> const T * { return ptr_.get(); } + + explicit operator bool() const noexcept { return static_cast(ptr_); } + +private: + static auto clone_of_(const std::unique_ptr &src) -> std::unique_ptr { + if (!src) { + return nullptr; + } + if constexpr (requires { + { src->clone() } -> std::convertible_to>; + }) { + return src->clone(); + } + else { + return std::make_unique(*src); + } + } + + std::unique_ptr ptr_; +}; + +/// Construct a `value_ptr` pointee in place, as `make_unique` does. +template +auto make_value(Args &&...args) -> value_ptr { + return value_ptr(std::make_unique(std::forward(args)...)); +} + +} // namespace monoprop diff --git a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl index 3b529777..8aeaf1e2 100644 --- a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl +++ b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl @@ -133,9 +133,8 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope basis, /*partitions=*/1); }; - partition_group_ = std::make_unique>(static_cast(n_partitions), - factory, - comm); + partition_group_ = + make_value>(static_cast(n_partitions), factory, comm); return; } @@ -166,7 +165,7 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope const size_t expected_local_terms = std::max(1, op.size() / std::max(1, num_ranks)); // Must run before the store: packed_inline_width_() derives the packed-row width from cutoff_fn_. regenerate_cutoff_fn_(); - mp_op_.store = std::make_unique>(packed_inline_width_()); + mp_op_.store = make_value>(packed_inline_width_()); mp_op_.store->reserve(expected_local_terms); // Store replaced: drop the stale lazy inverted index so it rebuilds against the new store. mp_op_.inverted_index_.reset(); @@ -187,30 +186,6 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope initialize_operator_caches_(); } -template -MonomialPropagator::~MonomialPropagator() = default; - -template -MonomialPropagator::MonomialPropagator(const MonomialPropagator &other) - : schrodinger_(other.schrodinger_), - comm_(other.comm_), - cutoff_fn_(other.cutoff_fn_), - mp_op_(other.mp_op_), - graph_(other.graph_), - matched_scratch_(other.matched_scratch_), - cutoff_(other.cutoff_), - lower_atol_(other.lower_atol_), - upper_atol_(other.upper_atol_), - core_term_(other.core_term_), - initial_operator_epoch_(other.initial_operator_epoch_), - logical_num_modes_(other.logical_num_modes_), - cutoff_type_(other.cutoff_type_), - basis_change_(other.basis_change_), - basis_(other.basis_), - partition_group_(other.partition_group_ - ? std::make_unique>(*other.partition_group_) - : nullptr) {} - template auto MonomialPropagator::resolve_partition_count_(size_t requested, mpi::Comm comm) -> size_t { if (requested >= 1) { diff --git a/cpp/monoprop/detail/operator/MPOperator.h b/cpp/monoprop/detail/operator/MPOperator.h index 6d4e70da..a64bcf24 100644 --- a/cpp/monoprop/detail/operator/MPOperator.h +++ b/cpp/monoprop/detail/operator/MPOperator.h @@ -28,6 +28,7 @@ #include "monoprop/TypeAliases.h" #include "monoprop/Utilities.h" +#include "monoprop/ValuePtr.h" #include "monoprop/detail/operator/InvertedIndex.h" #include "monoprop/detail/operator/OperatorIndex.h" @@ -60,9 +61,10 @@ class OperatorTermNotFound : public std::runtime_error { template struct MPOperator { - // The store is non-copyable/non-movable, so it is heap-owned by unique_ptr (keeping MPOperator - // itself cheaply movable). Always non-null. - std::unique_ptr> store = std::make_unique>(); + // The store is non-copyable/non-movable, so it is heap-owned (keeping MPOperator itself cheaply + // movable); value_ptr clones it through OperatorIndex::clone(), which is what lets MPOperator declare + // no special member of its own. Always non-null. + value_ptr> store = make_value>(); VecD op_coeffs = {}; // Only fully-paired terms score nonzero (see score_new_state_rows_), which on production models is // ~0.07% of the rows -- a dense vector here is 99.9% zeros. state_rows_ is strictly ascending: rows are @@ -79,21 +81,8 @@ struct MPOperator { Basis basis = Basis::Majorana; mutable std::optional> inverted_index_ = std::nullopt; - MPOperator() noexcept = default; - MPOperator(MPOperator &&) noexcept = default; - MPOperator &operator=(MPOperator &&) noexcept = default; - - MPOperator(const MPOperator &other) - : store(other.store->clone()), - op_coeffs(other.op_coeffs), - state_rows_(other.state_rows_), - state_vals_(other.state_vals_), - state_scored_rows_(other.state_scored_rows_), - state_coeffs(other.state_coeffs), - init_op_map(other.init_op_map), - initial_state(other.initial_state), - basis(other.basis), - inverted_index_(other.inverted_index_) {} + // Rule of Zero: every special member is implicit, so a member added above is copied and moved without + // an edit here. Copying deep-copies the store; moving steals it. auto size() const -> size_t { return store->size(); } diff --git a/cpp/tests/simulator_copy_tests.cpp b/cpp/tests/simulator_copy_tests.cpp index be79c160..3c236a1f 100644 --- a/cpp/tests/simulator_copy_tests.cpp +++ b/cpp/tests/simulator_copy_tests.cpp @@ -15,6 +15,7 @@ #include #include +#include #include "TestUtilities.h" #include "monoprop/MonomialPropagator.h" @@ -27,10 +28,14 @@ using namespace test_utils; using namespace monoprop; -// Copy assignment is deliberately deleted: the unique_ptr-owned store needs no assignment. +// The simulator declares no special member (the Rule of Zero), so the compiler supplies all six: the +// value_ptr members carry the deep copy. Moving is a real move here -- nothing user-declared suppresses it +// any more -- so these four assertions are what keeps a later hand-written special member from silently +// turning a move back into a deep copy. static_assert(std::is_copy_constructible_v>, "simulator must be copyable"); static_assert(std::is_move_constructible_v>, "simulator must stay movable"); -static_assert(!std::is_copy_assignable_v>, "copy assignment stays deleted"); +static_assert(std::is_copy_assignable_v>, "simulator must be copy-assignable"); +static_assert(std::is_move_assignable_v>, "simulator must be move-assignable"); BOOST_FIXTURE_TEST_CASE(copy_constructed_simulator_matches_energy, ExampleDataFix) { SimulatorConfig cfg{.comm = MPI_COMM_SELF}; @@ -108,3 +113,45 @@ BOOST_FIXTURE_TEST_CASE(copy_constructed_simulator_index_valid, ExampleDataFix) }); BOOST_TEST(all_found); } + +// Copy assignment came with the Rule of Zero: nothing declares it, so the compiler supplies it, and the +// value_ptr members make it as deep as construction. The source must survive it intact. +BOOST_FIXTURE_TEST_CASE(copy_assigned_simulator_is_an_independent_deep_copy, ExampleDataFix) { + SimulatorConfig cfg{.comm = MPI_COMM_SELF}; + auto sim = build_simulator(data, cfg); + sim.build_graph(data.majoranas, data.param_inds, data.gen_coeffs); + + auto target = build_simulator(data, cfg); + BOOST_TEST(target.graph_layers() == 0u); + + target = sim; + BOOST_REQUIRE(target.graph_layers() == sim.graph_layers()); + BOOST_CHECK(&target.indexing() != &sim.indexing()); // cloned, not shared + + const double e_sim = sim.expectation_value_functional()(data.parameters); + const double e_target = target.expectation_value_functional()(data.parameters); + BOOST_CHECK_SMALL(e_sim - e_target, 1e-13); + + const size_t layers = sim.graph_layers(); + target.contract_partially(data.parameters, /*inplace=*/true); + BOOST_TEST(sim.graph_layers() == layers); +} + +// A move must move. Before the Rule of Zero the user-declared copy constructor suppressed the implicit +// move operations, so every "move" bound to the copy constructor and deep-copied the operator store; the +// carried-over store address is what shows that no longer happens. +BOOST_FIXTURE_TEST_CASE(move_constructed_simulator_steals_the_store, ExampleDataFix) { + SimulatorConfig cfg{.comm = MPI_COMM_SELF}; + auto sim = build_simulator(data, cfg); + sim.build_graph(data.majoranas, data.param_inds, data.gen_coeffs); + + const auto *store_before = &sim.indexing(); + const size_t layers_before = sim.graph_layers(); + const double e_before = sim.expectation_value_functional()(data.parameters); + + auto moved = std::move(sim); + + BOOST_CHECK(&moved.indexing() == store_before); + BOOST_TEST(moved.graph_layers() == layers_before); + BOOST_CHECK_SMALL(e_before - moved.expectation_value_functional()(data.parameters), 1e-13); +} diff --git a/cpp/tests/value_ptr_tests.cpp b/cpp/tests/value_ptr_tests.cpp new file mode 100644 index 00000000..c5811d52 --- /dev/null +++ b/cpp/tests/value_ptr_tests.cpp @@ -0,0 +1,120 @@ +// Copyright 2026 Algorithmiq +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// value_ptr is the vocabulary type behind the Rule of Zero in MonomialPropagator and MPOperator, so its +// two copy routes (T::clone() and T's copy constructor) and its const propagation are pinned here rather +// than only through the propagator's deep-copy tests. + +#include + +#include +#include + +#include "monoprop/ValuePtr.h" + +using monoprop::make_value; +using monoprop::value_ptr; + +namespace { + +// Copyable, no clone(): value_ptr must take the copy-constructor route. +struct Copyable { + int value; +}; + +// Non-copyable but cloneable, as OperatorIndex is: only the clone() route can duplicate it. +struct Cloneable { + int value; + + Cloneable(int v) : value(v) {} + Cloneable(const Cloneable &) = delete; + auto operator=(const Cloneable &) -> Cloneable & = delete; + + [[nodiscard]] auto clone() const -> std::unique_ptr { return std::make_unique(value); } +}; + +// A class holding one declares no special member of its own, and still gets all six. +struct Holder { + value_ptr held = make_value(0); +}; + +static_assert(std::is_copy_constructible_v); +static_assert(std::is_copy_assignable_v); +static_assert(std::is_move_constructible_v); +static_assert(std::is_move_assignable_v); + +} // namespace + +BOOST_AUTO_TEST_CASE(value_ptr_copy_uses_clone_when_available) { + auto original = make_value(7); + auto copy = original; + + BOOST_CHECK_EQUAL(copy->value, 7); + BOOST_CHECK(copy.get() != original.get()); // a separate pointee, not a shared one + + copy->value = 9; + BOOST_CHECK_EQUAL(original->value, 7); +} + +BOOST_AUTO_TEST_CASE(value_ptr_copy_falls_back_to_copy_construction) { + auto original = make_value(Copyable{.value = 3}); + auto copy = original; + + BOOST_CHECK_EQUAL(copy->value, 3); + BOOST_CHECK(copy.get() != original.get()); +} + +BOOST_AUTO_TEST_CASE(value_ptr_assignment_is_self_safe_and_deep) { + auto a = make_value(1); + auto b = make_value(2); + + a = b; + BOOST_CHECK_EQUAL(a->value, 2); + BOOST_CHECK(a.get() != b.get()); + + // Clone-before-release: assigning from itself must not read a freed pointee. Aliased so that + // -Wself-assign-overloaded does not reject the very case under test. + const value_ptr &alias = a; + const auto *before = a.get(); + a = alias; + BOOST_CHECK_EQUAL(a->value, 2); + BOOST_CHECK(a.get() != before); // the clone replaced the original, so the address moves +} + +BOOST_AUTO_TEST_CASE(value_ptr_move_steals_the_pointee) { + auto source = make_value(5); + const auto *owned = source.get(); + + auto moved = std::move(source); + BOOST_CHECK(moved.get() == owned); // no clone on the move path + BOOST_CHECK(!source); +} + +BOOST_AUTO_TEST_CASE(value_ptr_empty_copies_stay_empty) { + value_ptr empty; + BOOST_CHECK(!empty); + + auto copy = empty; + BOOST_CHECK(!copy); + + auto filled = make_value(4); + filled = empty; + BOOST_CHECK(!filled); +} + +BOOST_AUTO_TEST_CASE(value_ptr_propagates_const_to_the_pointee) { + static_assert(std::is_same_v &>()), Copyable &>); + static_assert(std::is_same_v &>()), const Copyable &>); + static_assert(std::is_same_v &>().get()), const Copyable *>); +} diff --git a/cspell.json b/cspell.json index 4f86409b..f14eed7c 100644 --- a/cspell.json +++ b/cspell.json @@ -43,11 +43,13 @@ "Majorana", "Majoranas", "majoranic", + "memberwise", "memray", "microbenchmarks", "mpiexec", "oversubscribe", "Pauli", + "pointee", "Paulis", "qubit", "qubits", From 5a9bcd02534ebaaa2836ea0f2d48d3378aeaa00c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Mon, 17 Aug 2026 11:48:10 +0000 Subject: [PATCH 2/4] chore(cpp): remove unused virtual keyword --- cpp/include/monoprop/MonomialPropagator.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/include/monoprop/MonomialPropagator.h b/cpp/include/monoprop/MonomialPropagator.h index b740e455..dd162bd0 100644 --- a/cpp/include/monoprop/MonomialPropagator.h +++ b/cpp/include/monoprop/MonomialPropagator.h @@ -266,7 +266,7 @@ class MonomialPropagator { auto evolved_operator_terms(const VecD ¶meters, double atol) -> std::vector>>; - virtual auto update_initial_operator(const OperatorDict &op_dict) -> void { apply_initial_operator_(op_dict); } + auto update_initial_operator(const OperatorDict &op_dict) -> void { apply_initial_operator_(op_dict); } protected: static inline const auto ev_fn = [](const EvalRequest &request, From 0bb22c0630144df12204a25c0b006c35d1d6d826 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Mon, 17 Aug 2026 16:13:11 +0200 Subject: [PATCH 3/4] style: sort entries in cspell.json --- cspell.json | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/cspell.json b/cspell.json index f14eed7c..8a52fa36 100644 --- a/cspell.json +++ b/cspell.json @@ -6,6 +6,17 @@ "language": "en-GB", // words - list of words to be always considered correct "words": [ + "Eigenkets", + "Eikås", + "Fock", + "Hamiltonian", + "Hartree", + "Ising", + "Majorana", + "Majoranas", + "Pauli", + "Paulis", + "Remigio", "ancillas", "anticommutation", "anticommutativity", @@ -21,11 +32,8 @@ "conj", "ctest", "cutoff", - "inplace", - "Ising", "devcontainer", "devcontainers", - "Eigenkets", "eigenpairs", "eigenprojected", "eigenprojection", @@ -34,29 +42,21 @@ "eigensolvers", "eigensystem", "eigenvector", - "Eikås", "fermionic", - "Fock", - "Hamiltonian", - "Hartree", + "inplace", "majorana", - "Majorana", - "Majoranas", "majoranic", "memberwise", "memray", "microbenchmarks", "mpiexec", "oversubscribe", - "Pauli", "pointee", - "Paulis", "qubit", "qubits", - "Remigio", - "tracemalloc", "schrodinger", "simulable", + "tracemalloc", "unnormalized", "unpackb" ], @@ -98,6 +98,7 @@ "majs", "micromamba", "minversion", + "monoprop", "msge", "mypy", "nanobind", @@ -126,7 +127,6 @@ "testpaths", "venv", "wyhash", - "xfail", - "monoprop" + "xfail" ] } From 1cdd32968a9d8ac0d651b8aa04813c86eaa7dfba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Tue, 18 Aug 2026 08:22:14 +0000 Subject: [PATCH 4/4] refactor: slightly better backport of std::indirect but of course there are memory footguns when going for rule-of-zero, since {copy,move} {CTOR,assignment} are back in the game automatically. --- AGENTS.md | 8 +- README.md | 6 + cpp/include/monoprop/MonomialPropagator.h | 6 +- cpp/monoprop/CMakeLists.txt | 2 +- cpp/monoprop/Indirect.h | 84 +++++++++++ cpp/monoprop/ValuePtr.h | 90 ------------ .../MonomialPropagator.inl | 19 ++- cpp/monoprop/detail/operator/MPOperator.h | 9 +- cpp/monoprop/detail/operator/OperatorIndex.h | 30 ++-- .../detail/partition/PartitionGroup.h | 2 +- cpp/tests/OpaqueIndirectHolder.cpp | 36 +++++ cpp/tests/OpaqueIndirectHolder.h | 50 +++++++ cpp/tests/indirect_tests.cpp | 134 ++++++++++++++++++ cpp/tests/mp_operator_tests.cpp | 4 +- cpp/tests/operator_index_tests.cpp | 35 ++--- cpp/tests/partition_equivalence_tests.cpp | 18 ++- cpp/tests/simulator_copy_tests.cpp | 10 +- cpp/tests/value_ptr_tests.cpp | 120 ---------------- docs/content/docs/how-to-contribute.mdx | 19 +++ 19 files changed, 408 insertions(+), 274 deletions(-) create mode 100644 cpp/monoprop/Indirect.h delete mode 100644 cpp/monoprop/ValuePtr.h create mode 100644 cpp/tests/OpaqueIndirectHolder.cpp create mode 100644 cpp/tests/OpaqueIndirectHolder.h create mode 100644 cpp/tests/indirect_tests.cpp delete mode 100644 cpp/tests/value_ptr_tests.cpp diff --git a/AGENTS.md b/AGENTS.md index a424c394..06719c86 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,9 +25,11 @@ monoprop is a high-performance C++/Python hybrid library implementing Majorana a them accurate. - In prose docs (`docs/content/docs/**.mdx`) and Python docstrings, link to API symbols with the mkdocstrings-style `[Symbol][]` reference (or `[Display][fully.qualified.path]`) — never hard-code `/api/...` URLs. Do not backtick the name in the `[Symbol][]` form. See `docs/content/docs/documenting.mdx`. - Keep classes on the Rule of Zero: declare no destructor, copy or move member. A member that needs a - deep copy because it is heap-owned goes in `value_ptr` (`cpp/monoprop/ValuePtr.h`), which clones its - pointee — through `T::clone()` when the type has one — rather than in a `unique_ptr` plus a hand-written - copy constructor that has to name every other member and silently drops any member added later. + deep copy because it is heap-owned goes in `indirect` (`cpp/monoprop/Indirect.h`), the project's + allocator-free C++23 subset of C++26 `std::indirect`. Its pointee must be copy-constructible; copying + creates an independent pointee and moving transfers ownership. Use `std::optional>` when + absence is a domain state. `T` may be incomplete where the member is declared, but any holder special + member that constructs, copies, or destroys it must be defined where `T` is complete. - C++ comments: bare `//` for one-line comments, `/* */` for block comments. - Add Doxygen Qt-style documentation on all declarations in header files. Put documentation after members in enums, structs, classes. diff --git a/README.md b/README.md index 9bf340f0..1d11e7d4 100644 --- a/README.md +++ b/README.md @@ -152,6 +152,12 @@ All contributions require accepting the Individual CLA through CLA Assistant. If you are contributing on behalf of your employer, contact [cla@algorithmiq.fi](mailto:cla@algorithmiq.fi) to arrange a Corporate CLA. +C++ classes use Rule-of-Zero ownership. A heap-owned value member should use the +project's C++23 `monoprop::indirect` vocabulary type; use +`std::optional>` when the member can be absent. See the +[contributor guide](https://docs.monoprop.algorithmiq.tech/how-to-contribute#c-ownership) +for its opaque-type and completeness rules. + ## Documentation The documentation is built with [Fumadocs](https://fumadocs.dev/) and hosted at diff --git a/cpp/include/monoprop/MonomialPropagator.h b/cpp/include/monoprop/MonomialPropagator.h index dd162bd0..f137e56c 100644 --- a/cpp/include/monoprop/MonomialPropagator.h +++ b/cpp/include/monoprop/MonomialPropagator.h @@ -34,12 +34,12 @@ #include #include "monoprop/Evolution.h" +#include "monoprop/Indirect.h" #include "monoprop/MPFunctions.h" #include "monoprop/MPGraph.h" #include "monoprop/TypeAliases.h" #include "monoprop/Utilities.h" #include "monoprop/Validation.h" -#include "monoprop/ValuePtr.h" #include "monoprop/algebra/PauliAlgebra.h" #include "monoprop/detail/evolution/CosineRecompute.h" #include "monoprop/detail/mpi/MPICompat.h" @@ -314,8 +314,8 @@ class MonomialPropagator { // Intra-process partition runtime. Null ⇒ ordinary single-partition propagator; non-null ⇒ a partition facade // whose own mp_op_/graph_ are unused and every method fans out to the S partition propagators. - value_ptr> partition_group_; - // PartitionGroup rebinds a cloned partition's comm_ to its own transport during a deep copy. + std::optional>> partition_group_; + // PartitionGroup rebinds a copied partition's comm_ to its own transport during a deep copy. friend class detail::partition::PartitionGroup; // A facade's own graph_/mp_op_ are never populated, so handing them out would return plausible-looking diff --git a/cpp/monoprop/CMakeLists.txt b/cpp/monoprop/CMakeLists.txt index 3d8b93da..72f90899 100644 --- a/cpp/monoprop/CMakeLists.txt +++ b/cpp/monoprop/CMakeLists.txt @@ -87,11 +87,11 @@ target_sources( TYPE HEADERS FILES "${PROJECT_SOURCE_DIR}/cpp/monoprop/Bitset.h" + "${PROJECT_SOURCE_DIR}/cpp/monoprop/Indirect.h" "${PROJECT_SOURCE_DIR}/cpp/monoprop/MPGraphEncoding.h" "${PROJECT_SOURCE_DIR}/cpp/monoprop/TypeAliases.h" "${PROJECT_SOURCE_DIR}/cpp/monoprop/Utilities.h" "${PROJECT_SOURCE_DIR}/cpp/monoprop/Validation.h" - "${PROJECT_SOURCE_DIR}/cpp/monoprop/ValuePtr.h" ) target_compile_definitions( diff --git a/cpp/monoprop/Indirect.h b/cpp/monoprop/Indirect.h new file mode 100644 index 00000000..38c53e9f --- /dev/null +++ b/cpp/monoprop/Indirect.h @@ -0,0 +1,84 @@ +// Copyright 2026 Algorithmiq +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include +#include + +namespace monoprop { + +/// An allocator-free C++23 subset of C++26 `std::indirect`. +// +// The pointee has value semantics: copying copy-constructs a distinct T, while moving transfers the +// allocation and leaves the source valueless. T may be incomplete where indirect is named, but must +// be complete wherever an operation constructs, copies, or destroys the pointee. +// +// This subset deliberately omits allocators, comparisons, hashing, value assignment, and constructors +// that adopt an existing pointer. Copy assignment replaces the allocation rather than preserving its +// address, so T need only be copy-constructible, not copy-assignable. +template +class indirect { +public: + /// The owned value type. + using value_type = T; + + /// Construct a value-initialized `T`. + explicit indirect() : ptr_(std::make_unique()) {} + + /// Construct `T` directly from `args`. + template + explicit indirect(std::in_place_t, Args &&...args) : ptr_(std::make_unique(std::forward(args)...)) {} + + /// Copy-construct an independent `T`, or preserve a moved-from state. + indirect(const indirect &other) : ptr_(other.ptr_ ? std::make_unique(*other.ptr_) : nullptr) {} + + /// Replace the owned value with an independent copy of `other`. + auto operator=(const indirect &other) -> indirect & { + if (this != std::addressof(other)) { + indirect replacement(other); + ptr_.swap(replacement.ptr_); + } + return *this; + } + + /// Transfer ownership and leave `other` valueless. + indirect(indirect &&) noexcept = default; + + /// Transfer ownership and leave `other` valueless. + auto operator=(indirect &&) noexcept -> indirect & = default; + + /// Destroy the owned value, if present. + ~indirect() = default; + + /// Access the owned value; the object must not be valueless. + auto operator*() noexcept -> T & { return *ptr_; } + + /// Access the owned value; the object must not be valueless. + auto operator*() const noexcept -> const T & { return *ptr_; } + + /// Access the owned value; the object must not be valueless. + auto operator->() noexcept -> T * { return ptr_.get(); } + + /// Access the owned value; the object must not be valueless. + auto operator->() const noexcept -> const T * { return ptr_.get(); } + + /// Return whether ownership was transferred from this object. + [[nodiscard]] auto valueless_after_move() const noexcept -> bool { return ptr_ == nullptr; } + +private: + std::unique_ptr ptr_; +}; + +} // namespace monoprop diff --git a/cpp/monoprop/ValuePtr.h b/cpp/monoprop/ValuePtr.h deleted file mode 100644 index 570867ce..00000000 --- a/cpp/monoprop/ValuePtr.h +++ /dev/null @@ -1,90 +0,0 @@ -// Copyright 2026 Algorithmiq -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#pragma once - -#include -#include -#include - -namespace monoprop { - -/// A `unique_ptr` that copies its pointee instead of refusing to copy. -// Exclusive ownership with value semantics. It exists so that a class holding a heap member -- because -// the member is non-copyable, non-movable, or incomplete at that point -- needs no hand-written copy -// constructor or destructor, and so keeps its implicit move operations: the Rule of Zero. A hand-written -// copy constructor has to name every other member too, and silently default-initializes any member added -// later. -// -// Copying prefers `T::clone()` when the pointee declares one, and falls back to `T`'s copy constructor. -// A type that owns internal indices usually offers clone() precisely because it forbids copy construction. -// -// `T` may be incomplete where `value_ptr` is named: as with `unique_ptr`, each member body that needs -// `T` complete is instantiated only at its own point of use. -template -class value_ptr { -public: - value_ptr() = default; - - explicit value_ptr(std::unique_ptr owned) noexcept : ptr_(std::move(owned)) {} - - value_ptr(const value_ptr &other) : ptr_(clone_of_(other.ptr_)) {} - - auto operator=(const value_ptr &other) -> value_ptr & { - // Clone and replace, never T's assignment: T need not be assignable, and taking the clone before - // the old pointee is released makes self-assignment safe without a guard. - ptr_ = clone_of_(other.ptr_); - return *this; - } - - value_ptr(value_ptr &&) noexcept = default; - auto operator=(value_ptr &&) noexcept -> value_ptr & = default; - ~value_ptr() = default; - - // const propagates to the pointee, unlike unique_ptr's: the pointee is a value member here, not a - // pointer the object happens to hold. - auto operator*() -> T & { return *ptr_; } - auto operator*() const -> const T & { return *ptr_; } - auto operator->() -> T * { return ptr_.get(); } - auto operator->() const -> const T * { return ptr_.get(); } - auto get() -> T * { return ptr_.get(); } - auto get() const -> const T * { return ptr_.get(); } - - explicit operator bool() const noexcept { return static_cast(ptr_); } - -private: - static auto clone_of_(const std::unique_ptr &src) -> std::unique_ptr { - if (!src) { - return nullptr; - } - if constexpr (requires { - { src->clone() } -> std::convertible_to>; - }) { - return src->clone(); - } - else { - return std::make_unique(*src); - } - } - - std::unique_ptr ptr_; -}; - -/// Construct a `value_ptr` pointee in place, as `make_unique` does. -template -auto make_value(Args &&...args) -> value_ptr { - return value_ptr(std::make_unique(std::forward(args)...)); -} - -} // namespace monoprop diff --git a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl index 8aeaf1e2..49d3397e 100644 --- a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl +++ b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl @@ -133,8 +133,7 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope basis, /*partitions=*/1); }; - partition_group_ = - make_value>(static_cast(n_partitions), factory, comm); + partition_group_.emplace(std::in_place, static_cast(n_partitions), factory, comm); return; } @@ -165,7 +164,7 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope const size_t expected_local_terms = std::max(1, op.size() / std::max(1, num_ranks)); // Must run before the store: packed_inline_width_() derives the packed-row width from cutoff_fn_. regenerate_cutoff_fn_(); - mp_op_.store = make_value>(packed_inline_width_()); + mp_op_.store = indirect>(std::in_place, packed_inline_width_()); mp_op_.store->reserve(expected_local_terms); // Store replaced: drop the stale lazy inverted index so it rebuilds against the new store. mp_op_.inverted_index_.reset(); @@ -226,13 +225,13 @@ auto MonomialPropagator::resolve_partition_count_(size_t requested, mp template auto MonomialPropagator::for_each_partition_(const std::function &fn) -> void { - partition_group_->run_on_all([&](int r) { fn(partition_group_->partition(r)); }); + (**partition_group_).run_on_all([&](int r) { fn((**partition_group_).partition(r)); }); } template template auto MonomialPropagator::map_partitions_(Fn fn) -> std::vector { - return detail::partition::map_partitions(*partition_group_, fn); + return detail::partition::map_partitions(**partition_group_, fn); } template @@ -255,8 +254,8 @@ template template auto MonomialPropagator::fold_partitions_(Proj proj, Accumulate accumulate) const -> R { R total{}; - for (int r = 0; r < partition_group_->partition_count(); ++r) { - accumulate(total, proj(partition_group_->partition(r))); + for (int r = 0; r < (**partition_group_).partition_count(); ++r) { + accumulate(total, proj((**partition_group_).partition(r))); } return total; } @@ -269,7 +268,7 @@ auto MonomialPropagator::sum_partitions_(Proj proj) const -> R { template auto MonomialPropagator::first_partition_() const -> const MonomialPropagator & { - return partition_group_->partition(0); + return (**partition_group_).partition(0); } template @@ -982,7 +981,7 @@ auto MonomialPropagator::expectation_value_functional(std::optional>>( map_partitions_([&](MonomialPropagator &s) { return s.expectation_value_functional(pare_threshold); })); - auto *grp = partition_group_.get(); + auto *grp = std::addressof(**partition_group_); return [grp, fns](const VecD ¶ms) -> double { return detail::partition::collect_on_all(*grp, [&](int r) { return (*fns)[static_cast(r)](params); })[0]; @@ -997,7 +996,7 @@ auto MonomialPropagator::expectation_value_and_gradient_functional(std if (partition_group_) { auto fns = std::make_shared(const VecD &)>>>(map_partitions_( [&](MonomialPropagator &s) { return s.expectation_value_and_gradient_functional(pare_threshold); })); - auto *grp = partition_group_.get(); + auto *grp = std::addressof(**partition_group_); return [grp, fns](const VecD ¶ms) -> std::pair { return detail::partition::collect_on_all(*grp, [&](int r) { return (*fns)[static_cast(r)](params); })[0]; diff --git a/cpp/monoprop/detail/operator/MPOperator.h b/cpp/monoprop/detail/operator/MPOperator.h index a64bcf24..58c33648 100644 --- a/cpp/monoprop/detail/operator/MPOperator.h +++ b/cpp/monoprop/detail/operator/MPOperator.h @@ -26,9 +26,9 @@ #include #include +#include "monoprop/Indirect.h" #include "monoprop/TypeAliases.h" #include "monoprop/Utilities.h" -#include "monoprop/ValuePtr.h" #include "monoprop/detail/operator/InvertedIndex.h" #include "monoprop/detail/operator/OperatorIndex.h" @@ -61,10 +61,9 @@ class OperatorTermNotFound : public std::runtime_error { template struct MPOperator { - // The store is non-copyable/non-movable, so it is heap-owned (keeping MPOperator itself cheaply - // movable); value_ptr clones it through OperatorIndex::clone(), which is what lets MPOperator declare - // no special member of its own. Always non-null. - value_ptr> store = make_value>(); + // The store is non-movable, so it is heap-owned (keeping MPOperator itself cheaply movable). + // indirect copy-constructs an independent index, letting MPOperator declare no special members. + indirect> store{std::in_place}; VecD op_coeffs = {}; // Only fully-paired terms score nonzero (see score_new_state_rows_), which on production models is // ~0.07% of the rows -- a dense vector here is 99.9% zeros. state_rows_ is strictly ascending: rows are diff --git a/cpp/monoprop/detail/operator/OperatorIndex.h b/cpp/monoprop/detail/operator/OperatorIndex.h index c094c157..c793c407 100644 --- a/cpp/monoprop/detail/operator/OperatorIndex.h +++ b/cpp/monoprop/detail/operator/OperatorIndex.h @@ -73,25 +73,23 @@ class OperatorIndex { explicit OperatorIndex(size_t inline_width = kDefaultInlinePositions) : inline_width_(std::clamp(inline_width, 1, kMaxInlinePositions)), stride_(1 + inline_width_) {} - OperatorIndex(const OperatorIndex &) = delete; - OperatorIndex &operator=(const OperatorIndex &) = delete; - OperatorIndex(OperatorIndex &&) = delete; - OperatorIndex &operator=(OperatorIndex &&) = delete; - - // Called only on an idle store, so it needs no synchronization. - [[nodiscard]] auto clone() const -> std::unique_ptr { - auto out = std::make_unique(inline_width_); - out->rows_ = rows_; - out->size_ = size_; - out->overflow_ = overflow_; - out->reserve_index(table_.count); - for (const Slot &e : table_.slots) { + // Copying rebuilds the keyless index so every slot resolves against this object's row storage. + OperatorIndex(const OperatorIndex &src) + : rows_(src.rows_), + size_(src.size_), + inline_width_(src.inline_width_), + stride_(src.stride_), + overflow_(src.overflow_) { + reserve_index(src.table_.count); + for (const Slot &e : src.table_.slots) { if (e.idx != kEmptySlot) { - out->insert_slot_(e.idx, e.h); + insert_slot_(e.idx, e.h); } } - return out; } + OperatorIndex &operator=(const OperatorIndex &) = delete; + OperatorIndex(OperatorIndex &&) = delete; + OperatorIndex &operator=(OperatorIndex &&) = delete; [[nodiscard]] auto size() const -> size_t { return size_; } @@ -355,7 +353,7 @@ class OperatorIndex { auto reserve_index(size_t n) -> void { table_.rehash_to(slots_for_(n + 1)); } // Insert (idx, h) into the table with no duplicate probe — callers on this path insert provably distinct - // keys (⊕G-injective miss batches, clone re-insertion). + // keys (⊕G-injective miss batches, copy-constructor re-insertion). auto insert_slot_(TermIndex idx, uint32_t h) -> void { table_.rehash_if_needed(); size_t s = spread(h) & table_.mask; diff --git a/cpp/monoprop/detail/partition/PartitionGroup.h b/cpp/monoprop/detail/partition/PartitionGroup.h index fc3f64fd..cade37ff 100644 --- a/cpp/monoprop/detail/partition/PartitionGroup.h +++ b/cpp/monoprop/detail/partition/PartitionGroup.h @@ -154,7 +154,7 @@ class PartitionGroup { } // Under an MPI parent, find how many ranks share this host and which we are, so each co-located rank - // pins to a disjoint core block (see partition_cpusets). Collective over `parent`; clones copy the result. + // pins to a disjoint core block (see partition_cpusets). Collective over `parent`; copies reuse the result. auto discover_node_peers_() -> void { #ifdef monoprop_ENABLE_MPI if (parent_.kind == mpi::Comm::Kind::Mpi && mpi::size(parent_) > 1) { diff --git a/cpp/tests/OpaqueIndirectHolder.cpp b/cpp/tests/OpaqueIndirectHolder.cpp new file mode 100644 index 00000000..1420eecf --- /dev/null +++ b/cpp/tests/OpaqueIndirectHolder.cpp @@ -0,0 +1,36 @@ +// Copyright 2026 Algorithmiq +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "OpaqueIndirectHolder.h" + +#include + +namespace test_utils { + +struct OpaqueValue { + int value; +}; + +OpaqueIndirectHolder::OpaqueIndirectHolder(int value) : value_(std::in_place, value) {} +OpaqueIndirectHolder::OpaqueIndirectHolder(const OpaqueIndirectHolder &) = default; +auto OpaqueIndirectHolder::operator=(const OpaqueIndirectHolder &) -> OpaqueIndirectHolder & = default; +OpaqueIndirectHolder::OpaqueIndirectHolder(OpaqueIndirectHolder &&) noexcept = default; +auto OpaqueIndirectHolder::operator=(OpaqueIndirectHolder &&) noexcept -> OpaqueIndirectHolder & = default; +OpaqueIndirectHolder::~OpaqueIndirectHolder() = default; + +auto OpaqueIndirectHolder::value() const -> int { + return value_->value; +} + +} // namespace test_utils diff --git a/cpp/tests/OpaqueIndirectHolder.h b/cpp/tests/OpaqueIndirectHolder.h new file mode 100644 index 00000000..b10cf219 --- /dev/null +++ b/cpp/tests/OpaqueIndirectHolder.h @@ -0,0 +1,50 @@ +// Copyright 2026 Algorithmiq +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include "monoprop/Indirect.h" + +namespace test_utils { + +struct OpaqueValue; + +class OpaqueIndirectHolder { +public: + /// Construct an opaque value. + explicit OpaqueIndirectHolder(int value); + + /// Copy the opaque value. + OpaqueIndirectHolder(const OpaqueIndirectHolder &); + + /// Replace the opaque value with a copy. + auto operator=(const OpaqueIndirectHolder &) -> OpaqueIndirectHolder &; + + /// Move the opaque value. + OpaqueIndirectHolder(OpaqueIndirectHolder &&) noexcept; + + /// Replace the opaque value by moving it. + auto operator=(OpaqueIndirectHolder &&) noexcept -> OpaqueIndirectHolder &; + + /// Destroy the opaque value where its type is complete. + ~OpaqueIndirectHolder(); + + /// Return the stored value. + auto value() const -> int; + +private: + monoprop::indirect value_; +}; + +} // namespace test_utils diff --git a/cpp/tests/indirect_tests.cpp b/cpp/tests/indirect_tests.cpp new file mode 100644 index 00000000..bc7dfc64 --- /dev/null +++ b/cpp/tests/indirect_tests.cpp @@ -0,0 +1,134 @@ +// Copyright 2026 Algorithmiq +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include + +#include +#include +#include +#include +#include + +#include "OpaqueIndirectHolder.h" +#include "monoprop/Indirect.h" + +using monoprop::indirect; + +namespace { + +struct Value { + int number = 3; + std::string label = "default"; + + Value() = default; + Value(int number, std::string label) : number(number), label(std::move(label)) {} +}; + +struct ThrowOnCopy { + static inline bool should_throw = false; + int value; + + explicit ThrowOnCopy(int value) : value(value) {} + ThrowOnCopy(const ThrowOnCopy &other) : value(other.value) { + if (should_throw) { + throw std::runtime_error("copy failed"); + } + } +}; + +} // namespace + +BOOST_AUTO_TEST_CASE(indirect_default_and_in_place_construction_own_values) { + indirect default_value; + BOOST_CHECK_EQUAL(default_value->number, 3); + BOOST_CHECK_EQUAL(default_value->label, "default"); + + indirect initialized(std::in_place, 7, "in place"); + BOOST_CHECK_EQUAL(initialized->number, 7); + BOOST_CHECK_EQUAL(initialized->label, "in place"); + BOOST_CHECK(!initialized.valueless_after_move()); +} + +BOOST_AUTO_TEST_CASE(indirect_copy_construction_is_deep) { + indirect original(std::in_place, 5, "original"); + auto copy = original; + + BOOST_CHECK(std::addressof(*copy) != std::addressof(*original)); + copy->number = 9; + BOOST_CHECK_EQUAL(original->number, 5); +} + +BOOST_AUTO_TEST_CASE(indirect_copy_assignment_replaces_the_value_and_is_self_safe) { + indirect source(std::in_place, 4, "source"); + indirect target(std::in_place, 8, "target"); + + target = source; + BOOST_CHECK_EQUAL(target->number, 4); + BOOST_CHECK(std::addressof(*target) != std::addressof(*source)); + + const auto *before = std::addressof(*target); + const indirect &alias = target; + target = alias; + BOOST_CHECK(std::addressof(*target) == before); +} + +BOOST_AUTO_TEST_CASE(indirect_copy_assignment_has_strong_exception_safety) { + indirect source(std::in_place, 2); + indirect target(std::in_place, 1); + + ThrowOnCopy::should_throw = true; + BOOST_CHECK_THROW(target = source, std::runtime_error); + ThrowOnCopy::should_throw = false; + BOOST_CHECK_EQUAL(target->value, 1); +} + +BOOST_AUTO_TEST_CASE(indirect_move_transfers_the_allocation) { + indirect source(std::in_place, 6, "moved"); + const auto *address = std::addressof(*source); + + auto destination = std::move(source); + BOOST_CHECK(source.valueless_after_move()); + BOOST_CHECK(std::addressof(*destination) == address); + + indirect assigned; + assigned = std::move(destination); + BOOST_CHECK(destination.valueless_after_move()); + BOOST_CHECK(std::addressof(*assigned) == address); +} + +BOOST_AUTO_TEST_CASE(indirect_copy_preserves_a_valueless_state) { + indirect source; + auto owner = std::move(source); + auto copy = source; + + BOOST_CHECK(source.valueless_after_move()); + BOOST_CHECK(copy.valueless_after_move()); + BOOST_CHECK(!owner.valueless_after_move()); +} + +BOOST_AUTO_TEST_CASE(indirect_propagates_const_to_the_value) { + static_assert(std::is_same_v &>()), Value &>); + static_assert(std::is_same_v &>()), const Value &>); + static_assert(std::is_same_v &>().operator->()), const Value *>); +} + +BOOST_AUTO_TEST_CASE(indirect_supports_an_opaque_holder) { + test_utils::OpaqueIndirectHolder original(11); + auto copy = original; + BOOST_CHECK_EQUAL(copy.value(), 11); + + test_utils::OpaqueIndirectHolder assigned(3); + assigned = original; + BOOST_CHECK_EQUAL(assigned.value(), 11); +} diff --git a/cpp/tests/mp_operator_tests.cpp b/cpp/tests/mp_operator_tests.cpp index d0a4698d..433993ec 100644 --- a/cpp/tests/mp_operator_tests.cpp +++ b/cpp/tests/mp_operator_tests.cpp @@ -272,12 +272,12 @@ BOOST_AUTO_TEST_CASE(mp_operator_estimate_memory_usage_tracks_inverted_index_pre BOOST_CHECK_GT(after.inverted_index_bytes, 0U); // present arm } -BOOST_AUTO_TEST_CASE(mp_operator_copy_constructor_clones_store_and_coeffs) { +BOOST_AUTO_TEST_CASE(mp_operator_copy_constructor_copies_store_and_coeffs) { auto op = build_indexed_op({indices_to_bitset<8>({0, 1}), indices_to_bitset<8>({2, 3})}); op.initial_state = {0}; (void)op.sparse_state(); - detail::MPOperator<8> copy(op); // deep copy via clone() + detail::MPOperator<8> copy(op); BOOST_CHECK_EQUAL(copy.size(), op.size()); BOOST_CHECK_EQUAL(copy.state_scored_rows_, op.state_scored_rows_); BOOST_CHECK(copy.state_rows_ == op.state_rows_); diff --git a/cpp/tests/operator_index_tests.cpp b/cpp/tests/operator_index_tests.cpp index 486776c7..53706f3f 100644 --- a/cpp/tests/operator_index_tests.cpp +++ b/cpp/tests/operator_index_tests.cpp @@ -42,10 +42,11 @@ constexpr size_t N = 32; using Store = OperatorIndex; using MSet = Monomial; -// Owners hold the store by unique_ptr and share stable pointers into it, so it must stay -// non-copyable and non-movable; clone() is the only deep copy. +// Owners share stable pointers into the store. Copy construction performs a controlled deep rebuild, +// while assignment and moves remain forbidden because they could invalidate those pointers. static_assert(!std::is_move_constructible_v, "OperatorIndex must remain non-movable"); -static_assert(!std::is_copy_constructible_v, "OperatorIndex must remain non-copyable"); +static_assert(std::is_copy_constructible_v, "OperatorIndex must support deep copy construction"); +static_assert(!std::is_copy_assignable_v, "OperatorIndex must remain non-copy-assignable"); MSet bs(const VecZ &r) { return indices_to_bitset(r); @@ -108,42 +109,42 @@ BOOST_AUTO_TEST_CASE(index_survives_rehash_in_place) { BOOST_TEST(*f == 50u); } -BOOST_AUTO_TEST_CASE(clone_is_deep_and_independent) { +BOOST_AUTO_TEST_CASE(copy_is_deep_and_independent) { Store a(4); // non-default width must carry over a.push_back(bs({0, 3, 5})); a.emplace(bs({0, 3, 5}), 0); a.push_back(bs({1, 2})); a.emplace(bs({1, 2}), 1); - auto b = a.clone(); - BOOST_TEST(b->size() == 2u); - BOOST_TEST((b->row(0) == bs({0, 3, 5}))); - auto f = b->find(bs({1, 2})); + Store b(a); + BOOST_TEST(b.size() == 2u); + BOOST_TEST((b.row(0) == bs({0, 3, 5}))); + auto f = b.find(bs({1, 2})); BOOST_TEST(f.has_value()); BOOST_TEST(*f == 1u); a.push_back(bs({6, 7})); a.emplace(bs({6, 7}), 2); - BOOST_TEST(b->size() == 2u); - BOOST_TEST(!b->find(bs({6, 7})).has_value()); + BOOST_TEST(b.size() == 2u); + BOOST_TEST(!b.find(bs({6, 7})).has_value()); - // If the clone still referenced the source's rows, this find would read a->row(0) (now {8,9}) + // If the copy still referenced the source's rows, this find would read a.row(0) (now {8,9}) // and fail. a.set(0, bs({8, 9})); - auto g = b->find(bs({0, 3, 5})); + auto g = b.find(bs({0, 3, 5})); BOOST_TEST(g.has_value()); BOOST_TEST(*g == 0u); } -BOOST_AUTO_TEST_CASE(clone_preserves_overflow_rows) { +BOOST_AUTO_TEST_CASE(copy_preserves_overflow_rows) { Store a(2); // width 2; a 3-position row overflows losslessly a.push_back(bs({0, 1, 2})); a.emplace(bs({0, 1, 2}), 0); - auto b = a.clone(); - BOOST_TEST(b->popcount(0) == 3u); - BOOST_TEST((b->row(0) == bs({0, 1, 2}))); - BOOST_TEST(*b->find(bs({0, 1, 2})) == 0u); + Store b(a); + BOOST_TEST(b.popcount(0) == 3u); + BOOST_TEST((b.row(0) == bs({0, 1, 2}))); + BOOST_TEST(*b.find(bs({0, 1, 2})) == 0u); } // find_batch (the group-prefetch pipelined lookup) must be semantically identical to n independent diff --git a/cpp/tests/partition_equivalence_tests.cpp b/cpp/tests/partition_equivalence_tests.cpp index 9c8a984e..e18ccf8d 100644 --- a/cpp/tests/partition_equivalence_tests.cpp +++ b/cpp/tests/partition_equivalence_tests.cpp @@ -207,12 +207,28 @@ BOOST_AUTO_TEST_CASE(partition_deep_copy_matches) { sim.build_graph(data.majoranas, data.param_inds, data.gen_coeffs); const double e = sim.expectation_value(data.parameters); - MonomialPropagator copy(sim); // clones the partition group (fresh threads + ShmComm) + MonomialPropagator copy(sim); // copies the partition group (fresh threads + ShmComm) const double e_copy = copy.expectation_value(data.parameters); BOOST_CHECK_EQUAL(e, e_copy); BOOST_CHECK_EQUAL(sim.size(), copy.size()); } +BOOST_AUTO_TEST_CASE(partition_copy_assignment_handles_optional_engagement) { + const auto data = load_case_data("random_exact.msgpack"); + auto partitioned = majorana_sim(data, 4); + partitioned.build_graph(data.majoranas, data.param_inds, data.gen_coeffs); + const double expected = partitioned.expectation_value(data.parameters); + + auto single = majorana_sim(data, 1); + single = partitioned; + BOOST_CHECK_EQUAL(single.expectation_value(data.parameters), expected); + BOOST_CHECK_EQUAL(single.size(), partitioned.size()); + + auto ordinary = majorana_sim(data, 1); + single = ordinary; + BOOST_CHECK_EQUAL(single.size(), ordinary.size()); +} + constexpr size_t kNq = 6; auto pauli_sim(const std::map &obs, size_t partitions) -> MonomialPropagator { diff --git a/cpp/tests/simulator_copy_tests.cpp b/cpp/tests/simulator_copy_tests.cpp index 3c236a1f..2c2427c1 100644 --- a/cpp/tests/simulator_copy_tests.cpp +++ b/cpp/tests/simulator_copy_tests.cpp @@ -22,14 +22,14 @@ #include "monoprop/detail/mpi/MPICompat.h" // Copy-constructing a simulator must produce a fully independent deep copy -- the mechanism behind -// Python __deepcopy__. The operator store is non-copyable, so the copy rebuilds it via clone() and -// find()/indexing() have to work on the copy's own rows. The MPI communicator handle is shared. +// Python __deepcopy__. The operator-store copy rebuilds its index, so find()/indexing() have to work +// on the copy's own rows. The MPI communicator handle is shared. using namespace test_utils; using namespace monoprop; // The simulator declares no special member (the Rule of Zero), so the compiler supplies all six: the -// value_ptr members carry the deep copy. Moving is a real move here -- nothing user-declared suppresses it +// indirect members carry the deep copy. Moving is a real move here -- nothing user-declared suppresses it // any more -- so these four assertions are what keeps a later hand-written special member from silently // turning a move back into a deep copy. static_assert(std::is_copy_constructible_v>, "simulator must be copyable"); @@ -115,7 +115,7 @@ BOOST_FIXTURE_TEST_CASE(copy_constructed_simulator_index_valid, ExampleDataFix) } // Copy assignment came with the Rule of Zero: nothing declares it, so the compiler supplies it, and the -// value_ptr members make it as deep as construction. The source must survive it intact. +// indirect members make it as deep as construction. The source must survive it intact. BOOST_FIXTURE_TEST_CASE(copy_assigned_simulator_is_an_independent_deep_copy, ExampleDataFix) { SimulatorConfig cfg{.comm = MPI_COMM_SELF}; auto sim = build_simulator(data, cfg); @@ -126,7 +126,7 @@ BOOST_FIXTURE_TEST_CASE(copy_assigned_simulator_is_an_independent_deep_copy, Exa target = sim; BOOST_REQUIRE(target.graph_layers() == sim.graph_layers()); - BOOST_CHECK(&target.indexing() != &sim.indexing()); // cloned, not shared + BOOST_CHECK(&target.indexing() != &sim.indexing()); // copied, not shared const double e_sim = sim.expectation_value_functional()(data.parameters); const double e_target = target.expectation_value_functional()(data.parameters); diff --git a/cpp/tests/value_ptr_tests.cpp b/cpp/tests/value_ptr_tests.cpp deleted file mode 100644 index c5811d52..00000000 --- a/cpp/tests/value_ptr_tests.cpp +++ /dev/null @@ -1,120 +0,0 @@ -// Copyright 2026 Algorithmiq -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// value_ptr is the vocabulary type behind the Rule of Zero in MonomialPropagator and MPOperator, so its -// two copy routes (T::clone() and T's copy constructor) and its const propagation are pinned here rather -// than only through the propagator's deep-copy tests. - -#include - -#include -#include - -#include "monoprop/ValuePtr.h" - -using monoprop::make_value; -using monoprop::value_ptr; - -namespace { - -// Copyable, no clone(): value_ptr must take the copy-constructor route. -struct Copyable { - int value; -}; - -// Non-copyable but cloneable, as OperatorIndex is: only the clone() route can duplicate it. -struct Cloneable { - int value; - - Cloneable(int v) : value(v) {} - Cloneable(const Cloneable &) = delete; - auto operator=(const Cloneable &) -> Cloneable & = delete; - - [[nodiscard]] auto clone() const -> std::unique_ptr { return std::make_unique(value); } -}; - -// A class holding one declares no special member of its own, and still gets all six. -struct Holder { - value_ptr held = make_value(0); -}; - -static_assert(std::is_copy_constructible_v); -static_assert(std::is_copy_assignable_v); -static_assert(std::is_move_constructible_v); -static_assert(std::is_move_assignable_v); - -} // namespace - -BOOST_AUTO_TEST_CASE(value_ptr_copy_uses_clone_when_available) { - auto original = make_value(7); - auto copy = original; - - BOOST_CHECK_EQUAL(copy->value, 7); - BOOST_CHECK(copy.get() != original.get()); // a separate pointee, not a shared one - - copy->value = 9; - BOOST_CHECK_EQUAL(original->value, 7); -} - -BOOST_AUTO_TEST_CASE(value_ptr_copy_falls_back_to_copy_construction) { - auto original = make_value(Copyable{.value = 3}); - auto copy = original; - - BOOST_CHECK_EQUAL(copy->value, 3); - BOOST_CHECK(copy.get() != original.get()); -} - -BOOST_AUTO_TEST_CASE(value_ptr_assignment_is_self_safe_and_deep) { - auto a = make_value(1); - auto b = make_value(2); - - a = b; - BOOST_CHECK_EQUAL(a->value, 2); - BOOST_CHECK(a.get() != b.get()); - - // Clone-before-release: assigning from itself must not read a freed pointee. Aliased so that - // -Wself-assign-overloaded does not reject the very case under test. - const value_ptr &alias = a; - const auto *before = a.get(); - a = alias; - BOOST_CHECK_EQUAL(a->value, 2); - BOOST_CHECK(a.get() != before); // the clone replaced the original, so the address moves -} - -BOOST_AUTO_TEST_CASE(value_ptr_move_steals_the_pointee) { - auto source = make_value(5); - const auto *owned = source.get(); - - auto moved = std::move(source); - BOOST_CHECK(moved.get() == owned); // no clone on the move path - BOOST_CHECK(!source); -} - -BOOST_AUTO_TEST_CASE(value_ptr_empty_copies_stay_empty) { - value_ptr empty; - BOOST_CHECK(!empty); - - auto copy = empty; - BOOST_CHECK(!copy); - - auto filled = make_value(4); - filled = empty; - BOOST_CHECK(!filled); -} - -BOOST_AUTO_TEST_CASE(value_ptr_propagates_const_to_the_pointee) { - static_assert(std::is_same_v &>()), Copyable &>); - static_assert(std::is_same_v &>()), const Copyable &>); - static_assert(std::is_same_v &>().get()), const Copyable *>); -} diff --git a/docs/content/docs/how-to-contribute.mdx b/docs/content/docs/how-to-contribute.mdx index afc88041..60d9cadd 100644 --- a/docs/content/docs/how-to-contribute.mdx +++ b/docs/content/docs/how-to-contribute.mdx @@ -43,6 +43,25 @@ To run all pre-commit checks manually: prek run --all ``` +## C++ ownership + +Keep owning classes on the Rule of Zero. Use `monoprop::indirect` from +`monoprop/Indirect.h` for a heap-owned value that must be copied deeply, and use +`std::optional>` when absence is part of the domain model. +The pointee must be copy-constructible. Copying the wrapper constructs an +independent pointee; moving it transfers the allocation and leaves the source +valueless. + +`T` may be forward-declared where `indirect` is named. As with C++26 +`std::indirect`, however, operations that construct, copy, or destroy the pointee +require `T` to be complete. For an opaque member, declare the holder's special +members in its header and default them in the implementation file after the +pointee definition. + +This C++23 subset intentionally omits allocator support, comparisons, hashing, +value assignment, pointer adoption, and initializer-list overloads. Its copy +assignment replaces the allocation and does not preserve the pointee's address. + ## Documentation Building the documentation locally requires [npm](https://docs.npmjs.com/), the Node.js package manager. Once npm is available, you can run: