diff --git a/AGENTS.md b/AGENTS.md index a932c4ab..06719c86 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,12 @@ 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 `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 f80f3e87..f137e56c 100644 --- a/cpp/include/monoprop/MonomialPropagator.h +++ b/cpp/include/monoprop/MonomialPropagator.h @@ -34,6 +34,7 @@ #include #include "monoprop/Evolution.h" +#include "monoprop/Indirect.h" #include "monoprop/MPFunctions.h" #include "monoprop/MPGraph.h" #include "monoprop/TypeAliases.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}; @@ -274,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, @@ -322,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. - std::unique_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 b18c8b33..72f90899 100644 --- a/cpp/monoprop/CMakeLists.txt +++ b/cpp/monoprop/CMakeLists.txt @@ -87,6 +87,7 @@ 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" 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/detail/monomial_propagator/MonomialPropagator.inl b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl index 117590ec..4ecc3881 100644 --- a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl +++ b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl @@ -133,9 +133,7 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope basis, /*partitions=*/1); }; - partition_group_ = std::make_unique>(static_cast(n_partitions), - factory, - comm); + partition_group_.emplace(std::in_place, static_cast(n_partitions), factory, comm); return; } @@ -166,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 = std::make_unique>(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(); @@ -187,30 +185,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) { @@ -251,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 @@ -280,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; } @@ -294,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 @@ -1007,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]; @@ -1022,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 6d4e70da..58c33648 100644 --- a/cpp/monoprop/detail/operator/MPOperator.h +++ b/cpp/monoprop/detail/operator/MPOperator.h @@ -26,6 +26,7 @@ #include #include +#include "monoprop/Indirect.h" #include "monoprop/TypeAliases.h" #include "monoprop/Utilities.h" #include "monoprop/detail/operator/InvertedIndex.h" @@ -60,9 +61,9 @@ 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-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 @@ -79,21 +80,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/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 be79c160..2c2427c1 100644 --- a/cpp/tests/simulator_copy_tests.cpp +++ b/cpp/tests/simulator_copy_tests.cpp @@ -15,22 +15,27 @@ #include #include +#include #include "TestUtilities.h" #include "monoprop/MonomialPropagator.h" #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; -// 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 +// 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"); 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 +// 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); + 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()); // copied, 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/cspell.json b/cspell.json index 4f86409b..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,27 +42,21 @@ "eigensolvers", "eigensystem", "eigenvector", - "Eikås", "fermionic", - "Fock", - "Hamiltonian", - "Hartree", + "inplace", "majorana", - "Majorana", - "Majoranas", "majoranic", + "memberwise", "memray", "microbenchmarks", "mpiexec", "oversubscribe", - "Pauli", - "Paulis", + "pointee", "qubit", "qubits", - "Remigio", - "tracemalloc", "schrodinger", "simulable", + "tracemalloc", "unnormalized", "unpackb" ], @@ -96,6 +98,7 @@ "majs", "micromamba", "minversion", + "monoprop", "msge", "mypy", "nanobind", @@ -124,7 +127,6 @@ "testpaths", "venv", "wyhash", - "xfail", - "monoprop" + "xfail" ] } 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: