Follow-ups to #593: iterator interop, checked Expr::at(), hash invalidation

[evaleev]

Follow-up to #593, which turned Expr into a proper range. Three defects that CI could not catch, plus regression tests for each, plus one performance restoration.

Each commit stands alone, so the perf commit (27f02258) can be dropped without touching the correctness fixes.

1. const/non-const iterators do not interoperate at all

ExprIteratorImpl's heterogeneous operator- / operator== / operator<=> read other.ptr_ of the other specialization, which is private, with no friend declaration anywhere. Every one of those overloads is a hard error the moment it is instantiated:

error: 'ptr_' is a private member of 'sequant::detail::ExprIteratorImpl<true>'

Nothing in tree currently mixes the two types, which is why master builds. But expr.begin() != expr.cend() does not compile, and neither does pairing sequant::cbegin(ExprPtr const&) (expr_algorithms.hpp:65) with the non-const sequant::end(ExprPtr&) (expr_algorithms.hpp:78). There was also no ExprIterator → ConstExprIterator conversion, so the const/non-const interop these overloads exist to provide was entirely non-functional.

Fixed by befriending all specializations, collapsing each <is_const>/<!is_const> overload pair into one member template, and adding a converting constructor (mutable → const only; it is a constructor template so it is never treated as a copy constructor). Also dropped operator-(difference_type, ExprIteratorImpl) — n - it is not a valid random-access-iterator expression and it silently computed it - n. The valid n + it counterpart stays.

2. Expr::at() lost its bounds check

Dropping ranges::view_interface also dropped its at(), which threw when the index was out of range. The replacement forwards to operator[], whose only guard is SEQUANT_ASSERT — a no-op unless SEQUANT_ASSERT_ENABLED is #defined. In a build configured with SEQUANT_ASSERT_BEHAVIOR=IGNORE, sum.at(p) (optimize/sum.cpp:118) degrades from a thrown exception to an out-of-bounds read returning a garbage ExprPtr&. The parameter also changed from a signed difference type to std::size_t, so at(-1) went from throwing to wrapping to SIZE_MAX.

back() had the same problem from the other end: at(size() - 1) on an empty Expr — every atom — computes at(SIZE_MAX).

at() now always checks and throws sequant::Exception (project convention; nothing in tree catches std::out_of_range from here). The throwing path is out of line so it does not bloat callers. operator[] keeps its assert-only check, now documented as unchecked.

3. Product::end_subexpr() no longer invalidates the memoized hash

Product::end_cursor() used to call reset_hash_value(); the end_subexpr() that replaced it does not. So *(--product.end()) = new_factor mutates a factor while leaving the memoized hash in place — which trips the *hash_value_ == compute_hash() assert in Product::memoizing_hash(), and with asserts disabled leaves a stale hash that makes static_equal() short-circuit to false for products that are in fact equal.

Sum::begin_subexpr() gained the reset the old Sum::begin_cursor() never had, which is the right call. Sum::end_subexpr() gets it too, so both ends of both containers agree.

4. Hot accessors moved back into the header (separate commit, 27f02258)

begin/end/cbegin/cend/size/empty/operator[]/at/front/back are one-line forwarders on the hottest paths in the library. Expr::is_atom() alone is called from visit_impl(), is_scalar(), is_cnumber(), ExprRange::next_atom() and the Wick/canonicalization code; out-of-line definitions turn each into a non-inlinable cross-TU call on top of the virtual dispatch they already pay for. empty() now compares begin/end rather than computing size() == 0, saving a pair of virtual calls.

This is a restoration, not a new decision — before the switch away from ranges::view_interface these were all header-inline (as CRTP base templates), and is_atom() was ranges::empty(*this).

Measured rather than asserted. Release/-O3, SEQUANT_ASSERT_BEHAVIOR=IGNORE, Apple clang 17, against the identical tree with these bodies moved back to expr.cpp — 3 interleaved rounds of 3 repetitions, comparing per-benchmark medians over sequant_benchmarks (canonicalize, simplify, rapid_simplify, spintrace, tensor_block, random_tensor_network):

benchmarks faster inline	53 of 53
median delta	−2.29%
mean delta	−2.26%
best / worst	−4.31% (rapid_simplify/1) / −0.09%

No code-size cost — the trivial bodies inline away rather than bloat:

artifact	inline	out-of-line
libSeQuant-symb.a	7 437 952 B	7 441 224 B
sequant_benchmarks	4 985 824 B	4 986 816 B

Mechanism, for the record: out-of-line, libSeQuant-symb.a alone carries 34 undefined cross-TU references to these accessors and emits 16 definitions for them; inline it has zero of either.

_{cc_full_derivation was excluded from the benchmark set: it segfaults, but it does so on unmodified master (3168a655) too, so it is unrelated to this branch — reported separately.}

Verification

Each new test fails without its fix:

test	without the fix
mixed const/non-const iteration	does not compile ('ptr_' is a private member, no viable conversion)
checked element access	REQUIRE_THROWS_AS(sum->at(2), Exception) fails with SEQUANT_ASSERT_BEHAVIOR=IGNORE
hash invalidation on mutable iteration	fails for both Product and Sum

Full unit suite run locally in two configurations:

• Debug, SEQUANT_ASSERT_BEHAVIOR=THROW: 6750 assertions in 62 test cases, all passed
• Release, SEQUANT_ASSERT_BEHAVIOR=IGNORE: 310895 assertions in 63 test cases, all passed

CI is green on all 16 checks — GCC 14 and clang, Debug and Release, sanitizers, Valgrind.

[Krzmbrzl]

I don't think it is good practice to move implementations into header files unless crucial for performance.

Enabling LTO seems like the better way to address potential performance bottlenecks from these sorts of things 🤔