Merge pull request #55 from bluescarni/pr/ranges_fixes

Ranges fixes

doc/custom_construct.rst

@@ -36,7 +36,7 @@ The invalid state
 
 A :cpp:class:`wrap` object is in the *invalid* state when it is empty, that is, it does not contain
 any value. The validity of a :cpp:class:`wrap` can be checked via the :cpp:func:`~wrap::is_invalid()`
-function.
+and :cpp:func:`is_valid()` functions.
 
 A :cpp:class:`wrap` object can become invalid in a variety of circumstances:
 
@@ -51,7 +51,7 @@ A :cpp:class:`wrap` object can become invalid in a variety of circumstances:
 The only allowed operations on an invalid :cpp:class:`wrap` are:
 
 - destruction,
-- the invocation of :cpp:func:`~wrap::is_invalid()`,
+- the invocation of :cpp:func:`~wrap::is_invalid()` and :cpp:func:`is_valid()`,
 - copy/move assignment from, and swapping with, a valid :cpp:class:`wrap`,
 - :ref:`emplacement <emplacement>`,
 - generic assignment.

doc/wrap.rst

@@ -101,8 +101,10 @@ The ``wrap`` class
       This function will first destroy the value in *w* (if *w* is not already in the :ref:`invalid state <invalid_state>`).
       It will then construct in *w* a value of type :cpp:type:`T` using the construction arguments :cpp:type:`Args`.
 
-      This function is enabled only if :cpp:type:`T` is not :cpp:class:`wrap` (that is, it is not possible to emplace
-      a :cpp:class:`wrap` into itself) and if an instance of :cpp:type:`T` can be constructed from :cpp:type:`Args`.
+      This function is enabled only if an instance of :cpp:type:`T` can be constructed from :cpp:type:`Args`.
+
+      Passing *w* as an argument in *args* (e.g., attempting to emplace *w* into itself) will lead to
+      undefined behaviour.
 
       This function is ``noexcept`` if all these conditions are satisfied:
 
@@ -126,6 +128,15 @@ The ``wrap`` class
 
       :return: ``true`` if *w* is currently employing static storage, ``false`` otherwise.
 
+.. cpp:function:: [[nodiscard]] friend bool is_valid(const wrap &w) noexcept
+
+   This function will return ``false`` if *w* is in the :ref:`invalid state <invalid_state>`,
+   ``true`` otherwise.
+
+   :param w: the input argument.
+
+   :return: the validity status for *w*.
+
 .. cpp:function:: template <typename IFace, auto Cfg> bool has_dynamic_storage(const wrap<IFace, Cfg> &w) noexcept
 
    Query the storage type of a :cpp:class:`wrap`.

include/tanuki/tanuki.hpp

@@ -1422,8 +1422,6 @@ class TANUKI_VISIBLE wrap : private detail::wrap_storage<IFace, Cfg.static_size,
     // are provided. This must be documented well.
     template <typename T, typename... U>
         requires std::default_initializable<ref_iface_t> &&
-                 // Forbid emplacing a wrap inside a wrap.
-                 (!std::same_as<T, wrap>) &&
                  // We must be able to construct the holder.
                  detail::holder_constructible_from<T, IFace, Cfg.semantics, U &&...>
     explicit wrap(std::in_place_type_t<T>, U &&...args) noexcept(noexcept(this->ctor_impl<T>(std::forward<U>(args)...))
@@ -1606,9 +1604,8 @@ class TANUKI_VISIBLE wrap : private detail::wrap_storage<IFace, Cfg.static_size,
     wrap &operator=(invalid_wrap_t) noexcept
     {
         if constexpr (Cfg.semantics == wrap_semantics::value) {
-            // Don't do anything if this is already
-            // in the invalid state.
-            if (!is_invalid(*this)) {
+            // Do something only if this is in a valid state.
+            if (is_valid(*this)) {
                 // Destroy the contained value.
                 destroy();
 
@@ -1697,15 +1694,13 @@ class TANUKI_VISIBLE wrap : private detail::wrap_storage<IFace, Cfg.static_size,
     // Emplacement.
     template <typename T, typename... Args>
         requires
-        // Forbid emplacing a wrap inside a wrap.
-        (!std::same_as<T, wrap>) &&
         // We must be able to construct the holder.
         detail::holder_constructible_from<T, IFace, Cfg.semantics, Args &&...>
-        friend void emplace(wrap &w, Args &&...args) noexcept(noexcept(w.ctor_impl<T>(std::forward<Args>(args)...)))
+    friend void emplace(wrap &w, Args &&...args) noexcept(noexcept(w.ctor_impl<T>(std::forward<Args>(args)...)))
     {
         if constexpr (Cfg.semantics == wrap_semantics::value) {
             // Destroy the value in w if necessary.
-            if (!is_invalid(w)) {
+            if (is_valid(w)) {
                 w.destroy();
             }
 
@@ -1737,7 +1732,7 @@ class TANUKI_VISIBLE wrap : private detail::wrap_storage<IFace, Cfg.static_size,
     // The invalid state can also be explicitly set by constructing/assigning
     // from invalid_wrap_t.
     // The only valid operations on an invalid object are:
-    // - invocation of is_invalid(),
+    // - invocation of is_invalid()/is_valid(),
     // - destruction,
     // - copy/move assignment from, and swapping with, a valid wrap,
     // - generic assignment,
@@ -1968,6 +1963,12 @@ template <typename Holder, typename U>
     }
 }
 
+template <typename IFace, auto Cfg>
+[[nodiscard]] bool is_valid(const wrap<IFace, Cfg> &w) noexcept
+{
+    return !is_invalid(w);
+}
+
 template <typename IFace, auto Cfg>
 bool has_dynamic_storage(const wrap<IFace, Cfg> &w) noexcept
 {
@@ -2140,7 +2141,6 @@ struct tracking_level<tanuki::detail::value_iface<IFace, tanuki::wrap_semantics:
 #endif
 
 #undef TANUKI_ABI_TAG_ATTR
-#undef TANUKI_NO_UNIQUE_ADDRESS
 #undef TANUKI_VISIBLE
 
 #endif

test/CMakeLists.txt

@@ -63,6 +63,7 @@ ADD_TANUKI_TESTCASE(forward_range)
 ADD_TANUKI_TESTCASE(bidirectional_range)
 ADD_TANUKI_TESTCASE(random_access_range)
 ADD_TANUKI_TESTCASE(time_series)
+ADD_TANUKI_TESTCASE(test_subrange)
 
 if(TANUKI_WITH_BOOST_S11N)
   add_library(fooable SHARED fooable.cpp)

test/ranges.hpp

@@ -186,25 +186,45 @@ template <typename Base, typename Holder, typename T, typename V, typename R, ty
 struct generic_range_iface_impl<Base, Holder, T, V, R, RR, CR, CRR, It> : public Base {
     It<V, R, RR> begin() final
     {
-        return make_generic_iterator<It>{}(begin_end_impl::b(getval<Holder>(this)));
+        using ud_iter_t = decltype(begin_end_impl::b(getval<Holder>(this)));
+
+        // NOTE: here we want to detect the case in which ud_iter_t is equal to It<V, R, RR>
+        // (that is, it is already a type-erased iterator of exactly the correct type).
+        // In that case, make_generic_iterator() would just perform a copy of the
+        // user-defined iterator instead of type-erasing
+        // it, and this would cause issues with the logic in the sentinel class which will
+        // assume that the type-erased iterator passed to the at_end() and distance_to_iter()
+        // functions contains a user-defined iterator of type ud_iter_t (and that will not be
+        // the case, leading to a runtime exception).
+        if constexpr (std::same_as<ud_iter_t, It<V, R, RR>>) {
+            return It<V, R, RR>(std::in_place_type<It<V, R, RR>>, begin_end_impl::b(getval<Holder>(this)));
+        } else {
+            return make_generic_iterator<It>{}(begin_end_impl::b(getval<Holder>(this)));
+        }
     }
     sentinel end() final
     {
-        using iter_t = decltype(begin_end_impl::b(getval<Holder>(this)));
-        using sentinel_t = decltype(begin_end_impl::e(getval<Holder>(this)));
+        using ud_iter_t = decltype(begin_end_impl::b(getval<Holder>(this)));
+        using ud_sentinel_t = decltype(begin_end_impl::e(getval<Holder>(this)));
 
-        return sentinel(sentinel_box<sentinel_t, iter_t>{begin_end_impl::e(getval<Holder>(this))});
+        return sentinel(sentinel_box<ud_sentinel_t, ud_iter_t>{begin_end_impl::e(getval<Holder>(this))});
     }
     It<V, CR, CRR> begin() const final
     {
-        return make_generic_iterator<It>{}(begin_end_impl::b(getval<Holder>(this)));
+        using ud_iter_t = decltype(begin_end_impl::b(getval<Holder>(this)));
+
+        if constexpr (std::same_as<ud_iter_t, It<V, CR, CRR>>) {
+            return It<V, CR, CRR>(std::in_place_type<It<V, CR, CRR>>, begin_end_impl::b(getval<Holder>(this)));
+        } else {
+            return make_generic_iterator<It>{}(begin_end_impl::b(getval<Holder>(this)));
+        }
     }
     [[nodiscard]] sentinel end() const final
     {
-        using iter_t = decltype(begin_end_impl::b(getval<Holder>(this)));
-        using sentinel_t = decltype(begin_end_impl::e(getval<Holder>(this)));
+        using ud_iter_t = decltype(begin_end_impl::b(getval<Holder>(this)));
+        using ud_sentinel_t = decltype(begin_end_impl::e(getval<Holder>(this)));
 
-        return sentinel(sentinel_box<sentinel_t, iter_t>{begin_end_impl::e(getval<Holder>(this))});
+        return sentinel(sentinel_box<ud_sentinel_t, ud_iter_t>{begin_end_impl::e(getval<Holder>(this))});
     }
 };
 

test/sentinel.hpp

@@ -63,7 +63,7 @@ concept with_ptrdiff_t_difference = requires(const T &a, const U &b) { static_ca
 // wrapped into an any_ref).
 template <typename S, typename It>
 struct sentinel_box {
-    S m_sentinel;
+    TANUKI_NO_UNIQUE_ADDRESS S m_sentinel;
 
     [[nodiscard]] bool at_end(const any_ref &r_it) const
     {

test/test_basics.cpp

@@ -207,6 +207,15 @@ TEST_CASE("basics")
 
     // Same on holder_align.
     REQUIRE(!with_holder_align<nonwrappable, any_iface>);
+
+    // In-place nested construction.
+    wrap_t w4(4.), w5(std::in_place_type<wrap_t>, w4);
+    REQUIRE(!noexcept(wrap_t(std::in_place_type<wrap_t>, w4)));
+    REQUIRE(value_ref<double>(value_ref<wrap_t>(w5)) == 4.);
+
+    wrap_t w8(large{.str = "foobar"}), w9(std::in_place_type<wrap_t>, std::move(w8));
+    REQUIRE(value_ref<large>(value_ref<wrap_t>(w9)).str == "foobar");
+    REQUIRE(is_invalid(w8));
 }
 
 TEST_CASE("assignment")

test/test_emplace.cpp

@@ -58,7 +58,8 @@ TEST_CASE("emplace")
 
     REQUIRE(emplaceable<wrap_t, int, int>);
     REQUIRE(!emplaceable<wrap_t, std::string>);
-    REQUIRE(!emplaceable<wrap_t, wrap_t, int>);
+    REQUIRE(emplaceable<wrap_t, wrap_t, int>);
+    REQUIRE(emplaceable<wrap_t, wrap_t, wrap_t>);
 
     emplace<int>(w1, 43);
 
@@ -98,6 +99,25 @@ TEST_CASE("emplace")
     REQUIRE(noexcept(emplace<int>(w1, 43)));
     REQUIRE(!noexcept(emplace<large>(w1)));
     REQUIRE(!noexcept(emplace<thrower>(w1, 33)));
+
+    // Nested emplacement.
+    emplace<wrap_t>(w1, 12);
+    REQUIRE(value_isa<wrap_t>(w1));
+    REQUIRE(value_ref<int>(value_ref<wrap_t>(w1)) == 12);
+
+    auto w2 = wrap_t(tanuki::invalid_wrap);
+    emplace<wrap_t>(w2, w1);
+    REQUIRE(value_isa<wrap_t>(w2));
+    REQUIRE(value_ref<int>(value_ref<wrap_t>(value_ref<wrap_t>(w2))) == 12);
+
+    auto w3 = wrap_t(tanuki::invalid_wrap);
+    emplace<wrap_t>(w3, 123.);
+    REQUIRE(value_isa<wrap_t>(w3));
+    REQUIRE(value_ref<double>(value_ref<wrap_t>(w3)) == 123.);
+
+    auto w4 = wrap_t(12);
+    emplace<wrap_t>(w4, tanuki::invalid_wrap);
+    REQUIRE(is_invalid(value_ref<wrap_t>(w4)));
 }
 
 // NOLINTEND(cert-err58-cpp,misc-use-anonymous-namespace,cppcoreguidelines-avoid-do-while)

test/test_subrange.cpp

@@ -0,0 +1,63 @@
+#include <algorithm>
+#include <functional>
+#include <initializer_list>
+#include <ranges>
+#include <vector>
+
+#include <tanuki/tanuki.hpp>
+
+#include "ranges.hpp"
+
+#include <catch2/catch_test_macros.hpp>
+
+// NOLINTBEGIN(cert-err58-cpp,misc-use-anonymous-namespace,cppcoreguidelines-avoid-do-while)
+
+// NOTE: the purpose of this test is to check that
+// the range type erasure machinery works properly
+// when type-erasing non-type-erased ranges that return
+// type-erased iterators (say that three times).
+//
+// Before fixing the implementation of type-erased ranges,
+// the following would happen:
+// - no type-erasure would take place in the begin()
+//   method of generic_range due to the use of make_generic_iterator(),
+//   which would detect that the user-defined iterator is already
+//   type-erased and which would thus return a copy of the user-defined
+//   iterator;
+// - type-erasure would however always happen in the end() implementation,
+//   and this would break the logic in the sentinel methods that need to
+//   access the user-defined iterator stored in the wrap.
+TEST_CASE("type erased subranges")
+{
+    std::vector<int> v = {1, 2, 3};
+
+    {
+        auto r1 = facade::make_random_access_range(std::ref(v));
+        auto r2 = std::ranges::subrange(r1.begin(), r1.begin() + 2);
+        auto r3 = facade::make_random_access_range(std::ref(r2));
+        REQUIRE(std::ranges::equal(r3, r2));
+    }
+
+    {
+        auto r1 = facade::make_random_access_range(std::ref(v));
+        auto r2 = std::ranges::subrange(r1.begin(), r1.end());
+        auto r3 = facade::make_random_access_range(std::ref(r2));
+        REQUIRE(std::ranges::equal(r3, v));
+    }
+
+    {
+        const auto r1 = facade::make_random_access_range(std::cref(v));
+        auto r2 = std::ranges::subrange(r1.begin(), r1.begin() + 2);
+        const auto r3 = facade::make_random_access_range(std::cref(r2));
+        REQUIRE(std::ranges::equal(r3, r2));
+    }
+
+    {
+        const auto r1 = facade::make_random_access_range(std::cref(v));
+        auto r2 = std::ranges::subrange(r1.begin(), r1.end());
+        const auto r3 = facade::make_random_access_range(std::cref(r2));
+        REQUIRE(std::ranges::equal(r3, v));
+    }
+}
+
+// NOLINTEND(cert-err58-cpp,misc-use-anonymous-namespace,cppcoreguidelines-avoid-do-while)