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

SeQuant/core/expressions/expr.cpp

@@ -26,23 +26,12 @@
 #include <range/v3/view/reverse.hpp>
 #include <range/v3/view/transform.hpp>
 
+#include <sstream>
 #include <thread>
 #include <vector>
 
 namespace sequant {
 
-ExprIterator Expr::begin() { return begin_subexpr(); }
-
-ExprIterator Expr::end() { return end_subexpr(); }
-
-ConstExprIterator Expr::begin() const { return begin_subexpr(); }
-
-ConstExprIterator Expr::end() const { return end_subexpr(); }
-
-ConstExprIterator Expr::cbegin() const { return begin_subexpr(); }
-
-ConstExprIterator Expr::cend() const { return end_subexpr(); }
-
 ExprIterator Expr::begin_subexpr() { return ExprIterator{}; }
 
 ExprIterator Expr::end_subexpr() { return ExprIterator{}; }
@@ -51,32 +40,13 @@ ConstExprIterator Expr::begin_subexpr() const { return ConstExprIterator{}; }
 
 ConstExprIterator Expr::end_subexpr() const { return ConstExprIterator{}; }
 
-std::size_t Expr::size() const { return end() - begin(); }
-
-bool Expr::empty() const { return size() == 0; }
-
-ExprPtr &Expr::operator[](std::size_t idx) {
-  SEQUANT_ASSERT(idx < size());
-  return begin()[idx];
-}
-
-const ExprPtr &Expr::operator[](std::size_t idx) const {
-  SEQUANT_ASSERT(idx < size());
-  return begin()[idx];
+void Expr::throw_out_of_range(std::size_t idx) const {
+  std::ostringstream oss;
+  oss << "Expr::at(" << idx << "): index out of range (size=" << size()
+      << ", type_name=" << type_name() << ")";
+  throw Exception(oss.str());
 }
 
-ExprPtr &Expr::at(std::size_t idx) { return (*this)[idx]; }
-
-const ExprPtr &Expr::at(std::size_t idx) const { return (*this)[idx]; }
-
-ExprPtr &Expr::front() { return at(0); }
-
-const ExprPtr &Expr::front() const { return at(0); }
-
-ExprPtr &Expr::back() { return at(size() - 1); }
-
-const ExprPtr &Expr::back() const { return at(size() - 1); }
-
 ExprPtr ExprPtr::clone() const & {
   if (!*this) return {};
   return ExprPtr(as_shared_ptr()->clone());

SeQuant/core/expressions/expr.hpp

@@ -362,35 +362,69 @@ class Expr : public std::enable_shared_from_this<Expr> {
 
   ///@}
 
-  ExprIterator begin();
-  ExprIterator end();
-  ConstExprIterator begin() const;
-  ConstExprIterator end() const;
-  ConstExprIterator cbegin() const;
-  ConstExprIterator cend() const;
+  // N.B. these are deliberately defined inline: they sit on the hottest paths
+  // in the library (Expr::is_atom(), Expr::visit(), canonicalization, ...) and
+  // out-of-line definitions would turn each of them into a non-inlinable
+  // cross-TU call on top of the virtual dispatch they already pay for
+  ExprIterator begin() { return begin_subexpr(); }
+  ExprIterator end() { return end_subexpr(); }
+  ConstExprIterator begin() const { return begin_subexpr(); }
+  ConstExprIterator end() const { return end_subexpr(); }
+  ConstExprIterator cbegin() const { return begin_subexpr(); }
+  ConstExprIterator cend() const { return end_subexpr(); }
 
   virtual ExprIterator begin_subexpr();
   virtual ExprIterator end_subexpr();
   virtual ConstExprIterator begin_subexpr() const;
   virtual ConstExprIterator end_subexpr() const;
 
-  std::size_t size() const;
+  std::size_t size() const {
+    return static_cast<std::size_t>(end_subexpr() - begin_subexpr());
+  }
 
-  bool empty() const;
+  bool empty() const { return begin_subexpr() == end_subexpr(); }
 
-  ExprPtr &operator[](std::size_t idx);
-  const ExprPtr &operator[](std::size_t idx) const;
+  /// unchecked element access
+  /// @note the bounds check is only performed if `SEQUANT_ASSERT_ENABLED` is
+  ///       #defined; use at() for a bounds check that is always performed
+  ExprPtr &operator[](std::size_t idx) {
+    SEQUANT_ASSERT(idx < size());
+    return begin_subexpr()[static_cast<std::ptrdiff_t>(idx)];
+  }
+  /// @copydoc operator[](std::size_t)
+  const ExprPtr &operator[](std::size_t idx) const {
+    SEQUANT_ASSERT(idx < size());
+    return begin_subexpr()[static_cast<std::ptrdiff_t>(idx)];
+  }
 
-  ExprPtr &at(std::size_t idx);
-  const ExprPtr &at(std::size_t idx) const;
+  /// checked element access
+  /// @throw Exception if @p idx is not less than size()
+  ExprPtr &at(std::size_t idx) {
+    if (idx >= size()) throw_out_of_range(idx);
+    return begin_subexpr()[static_cast<std::ptrdiff_t>(idx)];
+  }
+  /// @copydoc at(std::size_t)
+  const ExprPtr &at(std::size_t idx) const {
+    if (idx >= size()) throw_out_of_range(idx);
+    return begin_subexpr()[static_cast<std::ptrdiff_t>(idx)];
+  }
 
-  ExprPtr &front();
-  const ExprPtr &front() const;
+  /// @throw Exception if this Expr is empty (e.g. is an atom)
+  ExprPtr &front() { return at(0); }
+  /// @copydoc front()
+  const ExprPtr &front() const { return at(0); }
 
-  ExprPtr &back();
-  const ExprPtr &back() const;
+  /// @throw Exception if this Expr is empty (e.g. is an atom)
+  ExprPtr &back() { return at(size() - 1); }
+  /// @copydoc back()
+  const ExprPtr &back() const { return at(size() - 1); }
 
  private:
+  /// reports an out-of-range access by at()/front()/back()
+  /// @note deliberately out-of-line so that the (cold) throwing path does not
+  ///       bloat the inlined callers
+  [[noreturn]] void throw_out_of_range(std::size_t idx) const;
+
   template <
       typename E, typename Visitor,
       typename = std::enable_if_t<std::is_same_v<std::remove_cvref_t<E>, Expr>>>

SeQuant/core/expressions/expr_iterator.hpp

@@ -4,6 +4,7 @@
 #include <SeQuant/core/utility/macros.hpp>
 
 #include <compare>
+#include <concepts>
 #include <iterator>
 #include <type_traits>
 
@@ -27,6 +28,16 @@ class ExprIteratorImpl {
 
   explicit ExprIteratorImpl(pointer ptr = nullptr) : ptr_(ptr) {}
 
+  /// converting constructor: a mutable iterator converts to a const iterator
+  /// (but not the other way around)
+  /// @note this is a constructor *template* on purpose: that way it is never
+  ///       considered a copy constructor and thus never suppresses the
+  ///       implicitly-declared one
+  template <bool other_is_const>
+    requires(is_const && !other_is_const)
+  ExprIteratorImpl(const ExprIteratorImpl<other_is_const> &other)
+      : ptr_(other.ptr_) {}
+
   ExprIteratorImpl &operator+=(difference_type val) {
     ptr_ += val;
     return *this;
@@ -65,11 +76,6 @@ class ExprIteratorImpl {
     return ExprIteratorImpl(it.ptr_ - val);
   }
 
-  friend ExprIteratorImpl operator-(difference_type val,
-                                    const ExprIteratorImpl &it) {
-    return ExprIteratorImpl(it.ptr_ - val);
-  }
-
   ExprIteratorImpl &operator--() {
     --ptr_;
     return *this;
@@ -93,17 +99,14 @@ class ExprIteratorImpl {
     return ptr_;
   }
 
-  difference_type operator-(const ExprIteratorImpl<is_const> &other) const {
-    return ptr_ - other.ptr_;
-  }
-  difference_type operator-(const ExprIteratorImpl<!is_const> &other) const {
+  template <bool other_is_const>
+  difference_type operator-(
+      const ExprIteratorImpl<other_is_const> &other) const {
     return ptr_ - other.ptr_;
   }
 
-  bool operator==(const ExprIteratorImpl<is_const> &other) const {
-    return ptr_ == other.ptr_;
-  }
-  bool operator==(const ExprIteratorImpl<!is_const> &other) const {
+  template <bool other_is_const>
+  bool operator==(const ExprIteratorImpl<other_is_const> &other) const {
     return ptr_ == other.ptr_;
   }
 
@@ -112,16 +115,19 @@ class ExprIteratorImpl {
     return *(ptr_ + offset);
   }
 
+  template <bool other_is_const>
   std::strong_ordering operator<=>(
-      const ExprIteratorImpl<is_const> &other) const {
-    return ptr_ <=> other.ptr_;
-  }
-  std::strong_ordering operator<=>(
-      const ExprIteratorImpl<!is_const> &other) const {
+      const ExprIteratorImpl<other_is_const> &other) const {
     return ptr_ <=> other.ptr_;
   }
 
  private:
+  // needed so that the const and the non-const specializations can access each
+  // other's ptr_ (see the converting constructor and the heterogeneous
+  // comparison/difference operators above)
+  template <bool>
+  friend class ExprIteratorImpl;
+
   pointer ptr_ = nullptr;
 };
 
@@ -135,6 +141,23 @@ static_assert(std::random_access_iterator<ExprIterator>);
 static_assert(std::bidirectional_iterator<ConstExprIterator>);
 static_assert(std::random_access_iterator<ConstExprIterator>);
 
+// mutable iterators must interoperate with (and convert to) const iterators,
+// but not vice versa
+static_assert(std::convertible_to<ExprIterator, ConstExprIterator>);
+static_assert(!std::convertible_to<ConstExprIterator, ExprIterator>);
+static_assert(std::equality_comparable_with<ExprIterator, ConstExprIterator>);
+static_assert(std::totally_ordered_with<ExprIterator, ConstExprIterator>);
+
+namespace detail {
+// `it - n` is a valid random-access-iterator expression, `n - it` is not
+// (unlike `n + it`, which is)
+template <typename It>
+concept subtractable_from_difference =
+    requires(It it, std::ptrdiff_t n) { n - it; };
+}  // namespace detail
+static_assert(!detail::subtractable_from_difference<ExprIterator>);
+static_assert(!detail::subtractable_from_difference<ConstExprIterator>);
+
 }  // namespace sequant
 
 #endif  // SEQUANT_EXPRESSIONS_EXPR_ITERATOR_HPP

SeQuant/core/expressions/product.hpp

@@ -398,6 +398,13 @@ class Product : public Expr {
   }
 
   ExprIterator end_subexpr() override {
+    // N.B. handing out a mutable iterator into factors_ invalidates the
+    // memoized hash, regardless of which end of the range it points at
+    // (`*(--end())` mutates just as `*begin()` does)
+    if (!factors_.empty()) {
+      reset_hash_value();
+    }
+
     return ExprIterator{factors_.data() + factors_.size()};
   }
 

SeQuant/core/expressions/sum.hpp

@@ -284,6 +284,13 @@ class Sum : public Expr {
   }
 
   ExprIterator end_subexpr() override {
+    // N.B. handing out a mutable iterator into summands_ invalidates the
+    // memoized hash, regardless of which end of the range it points at
+    // (`*(--end())` mutates just as `*begin()` does)
+    if (!summands_.empty()) {
+      reset_hash_value();
+    }
+
     return ExprIterator{summands_.data() + summands_.size()};
   }
 

tests/unit/test_expr.cpp

@@ -266,6 +266,82 @@ TEST_CASE("expr", "[elements]") {
     }
   }
 
+  SECTION("mixed const/non-const iteration") {
+    // N.B. Sum folds Constant summands together, so use Variables to get a
+    // Sum that actually holds two subexpressions
+    auto e = ex<Sum>(ExprPtrList{ex<Variable>(L"x"), ex<Variable>(L"y")});
+
+    // a mutable iterator converts to a const iterator ...
+    ConstExprIterator cit = e->begin();
+    REQUIRE(cit == e->cbegin());
+    // ... but not the other way around
+    static_assert(!std::is_convertible_v<ConstExprIterator, ExprIterator>);
+
+    // ... and the two compare/subtract heterogeneously, in either order
+    REQUIRE(e->begin() == e->cbegin());
+    REQUIRE(e->cbegin() == e->begin());
+    REQUIRE(e->begin() != e->cend());
+    REQUIRE(e->cend() != e->begin());
+    REQUIRE(e->begin() < e->cend());
+    REQUIRE(e->cend() > e->begin());
+    REQUIRE(e->cend() - e->begin() == 2);
+    REQUIRE(e->begin() - e->cend() == -2);
+
+    // same via the free functions, which return different iterator types
+    REQUIRE(sequant::cbegin(e) != sequant::end(e));
+    REQUIRE(sequant::end(e) - sequant::cbegin(e) == 2);
+  }
+
+  SECTION("checked element access") {
+    // N.B. unlike operator[], at()/front()/back() must throw regardless of
+    // whether SEQUANT_ASSERT is enabled, so this also pins down the behavior
+    // of builds configured with SEQUANT_ASSERT_BEHAVIOR=IGNORE
+    auto sum = ex<Sum>(ExprPtrList{ex<Variable>(L"x"), ex<Variable>(L"y")});
+    const Expr &const_sum = *sum;
+
+    REQUIRE(sum->at(0) == ex<Variable>(L"x"));
+    REQUIRE(sum->at(1) == ex<Variable>(L"y"));
+    REQUIRE(sum->front() == ex<Variable>(L"x"));
+    REQUIRE(sum->back() == ex<Variable>(L"y"));
+    REQUIRE_THROWS_AS(sum->at(2), Exception);
+    REQUIRE_THROWS_AS(const_sum.at(2), Exception);
+    // ... including an index that used to be a negative one
+    REQUIRE_THROWS_AS(sum->at(static_cast<std::size_t>(-1)), Exception);
+
+    // atoms are empty, so every element access throws (in particular,
+    // back() must not compute `at(size() - 1)` == `at(SIZE_MAX)` unchecked)
+    auto atom = ex<Constant>(3);
+    const Expr &const_atom = *atom;
+    REQUIRE(atom->empty());
+    REQUIRE(atom->size() == 0);
+    REQUIRE_THROWS_AS(atom->at(0), Exception);
+    REQUIRE_THROWS_AS(atom->front(), Exception);
+    REQUIRE_THROWS_AS(atom->back(), Exception);
+    REQUIRE_THROWS_AS(const_atom.front(), Exception);
+    REQUIRE_THROWS_AS(const_atom.back(), Exception);
+  }
+
+  SECTION("hash invalidation on mutable iteration") {
+    // handing out a mutable iterator must invalidate the memoized hash no
+    // matter which end of the range it points at: `*(--end())` mutates just
+    // as `*begin()` does
+    auto check = [](ExprPtr expr, bool via_end) {
+      const auto hash_before = expr->hash_value();
+      // this is the only accessor called before the mutation, so it alone is
+      // responsible for invalidating the memoized hash
+      auto it = via_end ? expr->end() : expr->begin();
+      *(via_end ? std::prev(it) : it) = ex<Variable>(L"mutated");
+      REQUIRE(expr->hash_value() != hash_before);
+    };
+
+    for (bool via_end : {false, true}) {
+      check(ex<Sum>(ExprPtrList{ex<Variable>(L"x"), ex<Variable>(L"y")}),
+            via_end);
+      check(ex<Product>(ExprPtrList{ex<Variable>(L"x"), ex<Variable>(L"y")}),
+            via_end);
+    }
+  }
+
   SECTION("constant") {
     const auto ex = std::make_shared<Constant>(2);
     REQUIRE(ex->value() == 2);