diff --git a/AGENTS.md b/AGENTS.md index cf2f2fd0..c21d54b1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -170,12 +170,17 @@ mp = MajoranaPropagator(operator, initial_state, cutoff=4) 2. Use C++23 syntax and idioms. 3. Use almost always auto style. 4. Use trailing return type syntax in function declarations. -5. Add a one-line `///` summary if the declaration is in `cpp/include/monoprop/`; elsewhere add a plain +5. Write a const/non-const accessor pair as one deducing-this member (`this Self &&self`, returning + `auto &`) instead of two bodies; reach for `std::forward_like` when the referent's const-ness + does not follow the owner's, as through a `unique_ptr`. Do **not** take the object parameter *by + value* to serve as an operator's working copy: that makes it a stack array, which loses NRVO and + trips `-fstack-protector-strong` — see the comment on `Bitset`'s bitwise operators. +6. Add a one-line `///` summary if the declaration is in `cpp/include/monoprop/`; elsewhere add a plain `//` note only where the code does not already say it. -6. Implement in the corresponding `.cpp` under `cpp/monoprop/`. -7. Add Python bindings in `src/monoprop/bindings/binder.h` -8. Regenerate bindings with `tools/generate-binders.py` -9. Test with both C++ and Python tests +7. Implement in the corresponding `.cpp` under `cpp/monoprop/`. +8. Add Python bindings in `src/monoprop/bindings/binder.h` +9. Regenerate bindings with `tools/generate-binders.py` +10. Test with both C++ and Python tests ## Documentation Maintenance Policy diff --git a/cpp/include/monoprop/MPGraph.h b/cpp/include/monoprop/MPGraph.h index e446c9d4..33459331 100644 --- a/cpp/include/monoprop/MPGraph.h +++ b/cpp/include/monoprop/MPGraph.h @@ -28,10 +28,9 @@ namespace monoprop { /// Ordered per-rank record of the evolution circuit, one Layer per generator. -class monoprop_EXPORT MPGraph { +class monoprop_EXPORT MPGraph : public LayerWindow { private: using LayerIterator = std::vector::iterator; - using ConstLayerIterator = std::vector::const_iterator; bool schrodinger_; std::vector layers_; @@ -41,18 +40,17 @@ class monoprop_EXPORT MPGraph { auto active_end_index() const -> size_t { return layers_.size(); } - auto active_begin_iterator() -> LayerIterator { - return layers_.begin() + static_cast(active_begin_index()); + // Deducing this: const-ness of the returned iterator follows the object, so neither body is doubled. + template + auto active_begin_iterator(this Self &&self) { + return self.layers_.begin() + static_cast(self.active_begin_index()); } - auto active_end_iterator() -> LayerIterator { return layers_.end(); } - - auto active_begin_iterator() const -> ConstLayerIterator { - return layers_.begin() + static_cast(active_begin_index()); + template + auto active_end_iterator(this Self &&self) { + return self.layers_.end(); } - auto active_end_iterator() const -> ConstLayerIterator { return layers_.end(); } - auto append_position() -> LayerIterator { return schrodinger_ ? active_begin_iterator() : active_end_iterator(); } auto append_layer(Layer layer) -> void { layers_.emplace(append_position(), std::move(layer)); } @@ -91,11 +89,12 @@ class monoprop_EXPORT MPGraph { auto layers() const -> size_t { return active_end_index() - active_begin_index(); } - auto get_layer(size_t layer_idx) -> Layer& { return layers_[checked_layer_offset(layer_idx)]; } - - auto get_layer(size_t layer_idx) const -> const Layer& { return layers_[checked_layer_offset(layer_idx)]; } - - auto get_layer_traversal(size_t layer_idx) const -> LayerTraversal { return get_layer(layer_idx).traversal(); } + /// The layer at `layer_idx` in build order; throws LayerIndexOutOfRange at or past the end. + // Deducing this: const-ness of the returned reference follows the object, so one body serves both. + template + auto get_layer(this Self &&self, size_t layer_idx) -> auto & { + return self.layers_[self.checked_layer_offset(layer_idx)]; + } /// Non-owning replay view over the active layers, in build order. auto replay_view() const -> MPGraphView { return {layers_, active_begin_index(), layers(), false}; } diff --git a/cpp/include/monoprop/MonomialPropagator.h b/cpp/include/monoprop/MonomialPropagator.h index b684ff58..61b741e9 100644 --- a/cpp/include/monoprop/MonomialPropagator.h +++ b/cpp/include/monoprop/MonomialPropagator.h @@ -116,13 +116,12 @@ class MonomialPropagator { } /// This rank's operator storage. Single-partition only — see require_single_partition_. - auto mp_op() -> detail::MPOperator & { - require_single_partition_("mp_op()"); - return mp_op_; - } - auto mp_op() const -> const detail::MPOperator & { - require_single_partition_("mp_op()"); - return mp_op_; + // Deducing this, so the partition guard is written once instead of once per const-ness. `Self` is left + // unconstrained: it deduces to a derived front-end when one calls this on itself, which is intended. + template + auto mp_op(this Self &&self) -> auto & { + self.require_single_partition_("mp_op()"); + return self.mp_op_; } // The breakdown fields are additive over the disjoint hash partitions, so a facade sums them. @@ -157,13 +156,12 @@ class MonomialPropagator { auto set_parameter_mapping(const VecZ ¶meter_mapping) -> void; /// This rank's monomial → coefficient index. Single-partition only — see require_single_partition_. - auto indexing() -> detail::OperatorIndex & { - require_single_partition_("indexing()"); - return *mp_op_.store; - } - auto indexing() const -> const detail::OperatorIndex & { - require_single_partition_("indexing()"); - return *mp_op_.store; + template + auto indexing(this Self &&self) -> auto & { + self.require_single_partition_("indexing()"); + // unique_ptr::operator* is const-qualified but yields a mutable referent, so the propagator's + // const-ness has to be re-applied by hand; plain `*store` would hand out a mutable index. + return std::forward_like(*self.mp_op_.store); } /// Per-layer (cos_inds, local_cycles, cross_rank_sin_send, cross_rank_sin_recv) for this diff --git a/cpp/monoprop/Bitset.h b/cpp/monoprop/Bitset.h index 6ad135a3..0c67259f 100644 --- a/cpp/monoprop/Bitset.h +++ b/cpp/monoprop/Bitset.h @@ -109,6 +109,10 @@ class Bitset { return *this; } + // These deliberately keep a named local and a by-reference object parameter rather than taking + // `this Bitset self` by value: a by-value object parameter is a stack array, which loses NRVO and + // (under -fstack-protector-strong, the platform default) puts a frame and a canary check on the + // library's hottest primitive. [[nodiscard]] constexpr auto operator~() const noexcept -> Bitset { Bitset r = *this; for (auto i = 0uz; i < kNumWords; ++i) @@ -177,8 +181,11 @@ class Bitset { } [[nodiscard]] static constexpr auto num_words() noexcept -> size_t { return kNumWords; } - [[nodiscard]] constexpr auto data() const noexcept -> const uint64_t * { return words_.data(); } - [[nodiscard]] constexpr auto data() noexcept -> uint64_t * { return words_.data(); } + // Deducing this: const-ness of the returned pointer follows the object. + template + [[nodiscard]] constexpr auto data(this Self &&self) noexcept -> auto * { + return self.words_.data(); + } [[nodiscard]] constexpr auto word(size_t i) const noexcept -> uint64_t { return words_[i]; } [[nodiscard]] constexpr auto find_first() const noexcept -> size_t { // NumBits if none diff --git a/cpp/monoprop/detail/graph/MPGraphViews.h b/cpp/monoprop/detail/graph/MPGraphViews.h index 80df2672..faa68d4b 100644 --- a/cpp/monoprop/detail/graph/MPGraphViews.h +++ b/cpp/monoprop/detail/graph/MPGraphViews.h @@ -68,9 +68,19 @@ struct GraphMemoryBreakdown final { } }; +// The layer-window vocabulary shared by MPGraph and its views: the deriving class supplies get_layer(), +// this supplies everything derivable from it. Deducing this rather than CRTP, so a deriving class need not +// name itself as a template argument, and the mixin stays an empty base. +struct LayerWindow { + template + auto get_layer_traversal(this const Self &self, size_t layer_idx) -> LayerTraversal { + return self.get_layer(layer_idx).traversal(); + } +}; + // `reverse` traverses the window newest-first (Schrödinger replay order). Non-owning — the layer vector // must outlive the view. -class MPGraphView { +class MPGraphView : public LayerWindow { public: MPGraphView(const std::vector &layers, size_t base, size_t count, bool reverse) : layers_(&layers), @@ -82,8 +92,6 @@ class MPGraphView { auto get_layer(size_t layer_idx) const -> const Layer & { return (*layers_)[checked_layer_offset(layer_idx)]; } - auto get_layer_traversal(size_t layer_idx) const -> LayerTraversal { return get_layer(layer_idx).traversal(); } - private: auto checked_layer_offset(size_t layer_idx) const -> size_t { if (layer_idx >= count_) { diff --git a/cpp/monoprop/detail/partition/PartitionGroup.h b/cpp/monoprop/detail/partition/PartitionGroup.h index 85000680..c93b6da8 100644 --- a/cpp/monoprop/detail/partition/PartitionGroup.h +++ b/cpp/monoprop/detail/partition/PartitionGroup.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include "monoprop/detail/mpi/Comm.h" @@ -106,8 +107,12 @@ class PartitionGroup { ~PartitionGroup() { stop_and_join_(); } auto partition_count() const -> int { return n_; } - auto partition(int s) -> MonomialPropagator & { return *partitions_[static_cast(s)]; } - auto partition(int s) const -> const MonomialPropagator & { return *partitions_[static_cast(s)]; } + // Deducing this: unique_ptr::operator* yields a mutable referent whatever the owner's const-ness, so + // forward_like re-applies this group's. + template + auto partition(this Self &&self, int s) -> auto & { + return std::forward_like(*self.partitions_[static_cast(s)]); + } // Run `body(partition_rank)` on all masters, block until every one finishes, then rethrow the first // exception raised (peers were released via poison, so a throw on one master never hangs the rest). diff --git a/cpp/tests/bitset_tests.cpp b/cpp/tests/bitset_tests.cpp index 6a3bac26..2666c429 100644 --- a/cpp/tests/bitset_tests.cpp +++ b/cpp/tests/bitset_tests.cpp @@ -20,12 +20,17 @@ #include #include #include +#include #include #include "monoprop/Bitset.h" using monoprop::Bitset; +// data() is one deducing-this member, so const-ness of the pointer is deduced rather than declared. +static_assert(std::is_same_v &>().data()), uint64_t *>); +static_assert(std::is_same_v &>().data()), const uint64_t *>); + namespace { template diff --git a/cpp/tests/mp_graph_tests.cpp b/cpp/tests/mp_graph_tests.cpp index 2f7e6bb2..403645fe 100644 --- a/cpp/tests/mp_graph_tests.cpp +++ b/cpp/tests/mp_graph_tests.cpp @@ -18,6 +18,7 @@ #include #include +#include #include #include "GraphBuildHarness.h" @@ -28,6 +29,10 @@ using test_utils::core_with_gate; using test_utils::graph_with_gates; using test_utils::layer_with_gate; +// get_layer() is one deducing-this member, so const-ness of the reference is deduced rather than declared. +static_assert(std::is_same_v().get_layer(0)), Layer &>); +static_assert(std::is_same_v().get_layer(0)), const Layer &>); + BOOST_AUTO_TEST_CASE(mp_graph_slice_graph_heisenberg_prefix_no_contract) { auto graph = graph_with_gates(/*schrodinger=*/false, 5); // layers_ = [0,1,2,3,4] auto sliced = graph.slice_graph(3, /*contract=*/false); diff --git a/cpp/tests/simulator_copy_tests.cpp b/cpp/tests/simulator_copy_tests.cpp index be79c160..6e600894 100644 --- a/cpp/tests/simulator_copy_tests.cpp +++ b/cpp/tests/simulator_copy_tests.cpp @@ -32,6 +32,19 @@ static_assert(std::is_copy_constructible_v>, "simulator mu static_assert(std::is_move_constructible_v>, "simulator must stay movable"); static_assert(!std::is_copy_assignable_v>, "copy assignment stays deleted"); +// mp_op()/indexing() are single deducing-this members. indexing() reaches its result through a +// unique_ptr, whose operator* hands back a mutable referent regardless of the owner's const-ness, so +// without the forward_like a const propagator would silently expose a writable index. +// `detail` is qualified: the two using-directives above make an unqualified one ambiguous. +static_assert( + std::is_same_v &>().mp_op()), monoprop::detail::MPOperator<8> &>); +static_assert(std::is_same_v &>().mp_op()), + const monoprop::detail::MPOperator<8> &>); +static_assert( + std::is_same_v &>().indexing()), monoprop::detail::OperatorIndex<8> &>); +static_assert(std::is_same_v &>().indexing()), + const monoprop::detail::OperatorIndex<8> &>); + BOOST_FIXTURE_TEST_CASE(copy_constructed_simulator_matches_energy, ExampleDataFix) { SimulatorConfig cfg{.comm = MPI_COMM_SELF}; auto sim = build_simulator(data, cfg);