diff --git a/include/beman/any_view/detail/iterator.hpp b/include/beman/any_view/detail/iterator.hpp index 8824fd2..b10c694 100644 --- a/include/beman/any_view/detail/iterator.hpp +++ b/include/beman/any_view/detail/iterator.hpp @@ -55,8 +55,10 @@ class iterator : public iterator_category_type, std::i static constexpr bool random_access = flag_is_set; static constexpr bool contiguous = flag_is_set; - using cache_type = - std::conditional_t, RValueRefT>, iter_cache_t, no_cache>; + using cache_type = std::conditional_t && + convertible_to_borrowed, RValueRefT>, + iter_cache_t, + no_cache>; using polymorphic_type = polymorphic_iterator; static constexpr bool has_cache = not std::is_same_v; @@ -262,9 +264,10 @@ class iterator : public iterator_category_type, std::i } [[nodiscard]] constexpr bool operator==(std::default_sentinel_t) const { - // sentinel comparison must dispatch for a contiguous iterator if constexpr (has_cache and not contiguous) { return cache_or_index == cache_type{}; + } else if constexpr (has_index) { + return dispatch>(poly, cache_or_index); } else { return dispatch(poly); } diff --git a/include/beman/any_view/detail/polymorphic.hpp b/include/beman/any_view/detail/polymorphic.hpp index 50fe3bb..9cdcf58 100644 --- a/include/beman/any_view/detail/polymorphic.hpp +++ b/include/beman/any_view/detail/polymorphic.hpp @@ -51,46 +51,37 @@ class basic_polymorphic { constexpr ~basic_polymorphic() { entry(destroy_t{})(storage); } constexpr basic_polymorphic& operator=(const basic_polymorphic& other) { - if (this == std::addressof(other)) { - return *this; + if (this != std::addressof(other)) { + *this = basic_polymorphic(other); } - std::destroy_at(this); - std::construct_at(this, other); return *this; } constexpr basic_polymorphic& operator=(basic_polymorphic&& other) noexcept { - if (this == std::addressof(other)) { - return *this; + if (this != std::addressof(other)) { + std::destroy_at(this); + std::construct_at(this, std::move(other)); } - std::destroy_at(this); - std::construct_at(this, std::move(other)); return *this; } template constexpr basic_polymorphic& operator=(AdaptorT&& adaptor) { - std::destroy_at(this); - std::construct_at(this, std::forward(adaptor)); - return *this; + return *this = basic_polymorphic(std::forward(adaptor)); } // converting assignment template ... OtherProtocolTs> constexpr basic_polymorphic& operator=(const basic_polymorphic& other) { - std::destroy_at(this); - std::construct_at(this, other); - return *this; + return *this = basic_polymorphic(other); } template ... OtherProtocolTs> constexpr basic_polymorphic& operator=(basic_polymorphic&& other) noexcept { - std::destroy_at(this); - std::construct_at(this, std::move(other)); - return *this; + return *this = basic_polymorphic(std::move(other)); } constexpr StorageT& get() & noexcept { return storage; } diff --git a/include/beman/any_view/detail/polymorphic_iterator.hpp b/include/beman/any_view/detail/polymorphic_iterator.hpp index 8cc7839..5caf163 100644 --- a/include/beman/any_view/detail/polymorphic_iterator.hpp +++ b/include/beman/any_view/detail/polymorphic_iterator.hpp @@ -48,6 +48,17 @@ struct sentinel_compare_t : unary_protocol { } }; +template +struct sentinel_compare_at_t : unary_protocol { + template + static bool fn(const T& self, DiffT offset); + + template + [[nodiscard]] static constexpr bool fn(const IteratorAdaptorT& adaptor, DiffT offset) { + return adaptor.iterator + offset == adaptor.sentinel; + } +}; + template struct dereference_t : unary_protocol { template @@ -243,22 +254,27 @@ struct rvalue_ref { template using rvalue_ref_t = typename rvalue_ref::type; -template -struct input_cache_protocol : inherit, iter_move_t, increment_t> {}; - -template - requires convertible_to_borrowed, RValueRefT> -struct input_cache_protocol : inherit, next_t> {}; - template struct input_protocol : inherit, destroy_t, - input_cache_protocol, + dereference_t, + iter_move_t, + increment_t, sentinel_compare_t> {}; template -struct forward_protocol - : inherit, copy_t, type_t, equality_compare_t> {}; +struct forward_cache_protocol : inherit<> {}; + +template + requires convertible_to_borrowed, RValueRefT> +struct forward_cache_protocol : inherit, next_t> {}; + +template +struct forward_protocol : inherit, + copy_t, + forward_cache_protocol, + type_t, + equality_compare_t> {}; template struct bidirectional_cache_protocol : inherit {}; @@ -278,6 +294,7 @@ struct random_access_cache_protocol : inherit struct random_access_protocol : inherit, random_access_cache_protocol, + sentinel_compare_at_t, three_way_compare_t, subtract_t> {}; diff --git a/tests/beman/any_view/CMakeLists.txt b/tests/beman/any_view/CMakeLists.txt index 788c65b..829809d 100644 --- a/tests/beman/any_view/CMakeLists.txt +++ b/tests/beman/any_view/CMakeLists.txt @@ -48,4 +48,4 @@ endfunction() beman_add_benchmark(all ${BENCHMARK_DETAIL_SOURCES}) beman_add_benchmark(take ${BENCHMARK_DETAIL_SOURCES}) -beman_add_tests(concepts constexpr iterator sfinae type_traits) +beman_add_tests(concepts constexpr iterator sfinae type_traits regressions) diff --git a/tests/beman/any_view/constexpr.test.cpp b/tests/beman/any_view/constexpr.test.cpp index be70b4a..78e2fd5 100644 --- a/tests/beman/any_view/constexpr.test.cpp +++ b/tests/beman/any_view/constexpr.test.cpp @@ -76,9 +76,9 @@ template non_trivially_copyable::non_trivially_copyable(const non_trivially_copyable&) = default; constexpr auto set_front(any_view view, int value) { - // lvalue reference uses cache object to fuse virtual dispatches - static_assert(sizeof(std::ranges::iterator_t>) == - sizeof(std::ranges::iterator_t>>) + sizeof(int*)); + // Only forward and stronger iterators cache lvalue references. + static_assert(sizeof(std::ranges::iterator_t>) == + sizeof(std::ranges::iterator_t>) + sizeof(int*)); auto& ref = view.front(); // even with cache object, lifetime of reference is not tied to lifetime of iterator diff --git a/tests/beman/any_view/detail/self_ref_forward_proxy_view.hpp b/tests/beman/any_view/detail/self_ref_forward_proxy_view.hpp new file mode 100644 index 0000000..96c4898 --- /dev/null +++ b/tests/beman/any_view/detail/self_ref_forward_proxy_view.hpp @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#pragma once + +#include +#include +#include +#include +#include + +struct self_ref_forward_proxy { + const int* pointer{}; + + operator int() const noexcept { return *pointer; } +}; + +static_assert(std::is_trivially_copyable_v); + +struct self_ref_forward_proxy_iterator { + using value_type = int; + using difference_type = std::ptrdiff_t; + using iterator_concept = std::forward_iterator_tag; + + mutable int value{}; + int stop{}; + + self_ref_forward_proxy_iterator() = default; + + self_ref_forward_proxy_iterator(int value, int stop) : value(value), stop(stop) {} + + self_ref_forward_proxy_iterator(const self_ref_forward_proxy_iterator&) = default; + + self_ref_forward_proxy_iterator& operator=(const self_ref_forward_proxy_iterator&) = default; + + self_ref_forward_proxy_iterator(self_ref_forward_proxy_iterator&& other) noexcept + : value(other.value), stop(other.stop) { + other.value = -777; + } + + self_ref_forward_proxy_iterator& operator=(self_ref_forward_proxy_iterator&& other) noexcept { + value = other.value; + stop = other.stop; + + other.value = -777; + return *this; + } + + self_ref_forward_proxy operator*() const noexcept { + // The proxy points into this iterator. + return {&value}; + } + + self_ref_forward_proxy_iterator& operator++() noexcept { + ++value; + return *this; + } + + self_ref_forward_proxy_iterator operator++(int) noexcept { + auto previous = *this; + ++*this; + return previous; + } + + friend bool operator==(const self_ref_forward_proxy_iterator&, const self_ref_forward_proxy_iterator&) = default; +}; + +static_assert(std::forward_iterator); + +struct self_ref_forward_proxy_view : std::ranges::view_base { + int first{}; + int stop{}; + + self_ref_forward_proxy_view() = default; + + self_ref_forward_proxy_view(int first, int stop) : first(first), stop(stop) {} + + self_ref_forward_proxy_iterator begin() const noexcept { return {first, stop}; } + + self_ref_forward_proxy_iterator end() const noexcept { return {stop, stop}; } +}; + +static_assert(std::ranges::forward_range); diff --git a/tests/beman/any_view/detail/self_ref_input_view.hpp b/tests/beman/any_view/detail/self_ref_input_view.hpp new file mode 100644 index 0000000..8138ab9 --- /dev/null +++ b/tests/beman/any_view/detail/self_ref_input_view.hpp @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#pragma once + +#include + +struct self_ref_input_iterator { + using value_type = int; + using difference_type = std::ptrdiff_t; + using iterator_concept = std::input_iterator_tag; + + mutable int value{}; + int stop{}; + + self_ref_input_iterator() = delete; + + self_ref_input_iterator(int value, int stop) : value(value), stop(stop) {} + + self_ref_input_iterator(const self_ref_input_iterator&) = delete; + self_ref_input_iterator& operator=(const self_ref_input_iterator&) = delete; + + self_ref_input_iterator(self_ref_input_iterator&& other) noexcept : value(other.value), stop(other.stop) { + other.value = -777; + } + + self_ref_input_iterator& operator=(self_ref_input_iterator&& other) noexcept { + value = other.value; + stop = other.stop; + + other.value = -777; + return *this; + } + + int& operator*() const noexcept { + // The returned reference points inside this iterator. + return value; + } + + self_ref_input_iterator& operator++() noexcept { + ++value; + return *this; + } + + void operator++(int) noexcept { ++*this; } + + friend bool operator==(const self_ref_input_iterator& iterator, std::default_sentinel_t) noexcept { + return iterator.value == iterator.stop; + } +}; + +static_assert(std::input_iterator); + +struct self_ref_input_view : std::ranges::view_base { + int first{}; + int stop{}; + + self_ref_input_view() = default; + + self_ref_input_view(int first, int stop) : first(first), stop(stop) {} + + self_ref_input_iterator begin() { return {first, stop}; } + + std::default_sentinel_t end() const noexcept { return {}; } +}; + +static_assert(std::ranges::input_range); diff --git a/tests/beman/any_view/detail/throwing_forward_view.hpp b/tests/beman/any_view/detail/throwing_forward_view.hpp new file mode 100644 index 0000000..9119fa5 --- /dev/null +++ b/tests/beman/any_view/detail/throwing_forward_view.hpp @@ -0,0 +1,86 @@ +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#pragma once + +#include +#include +#include +#include + +struct throwing_copy_error {}; + +struct throwing_forward_iterator { + using value_type = int; + using difference_type = std::ptrdiff_t; + using iterator_concept = std::forward_iterator_tag; + + inline static bool throw_on_copy = false; + inline static int live_count = 0; + + int* current{}; + + throwing_forward_iterator() noexcept { ++live_count; } + + explicit throwing_forward_iterator(int* current) noexcept : current(current) { ++live_count; } + + throwing_forward_iterator(const throwing_forward_iterator& other) : current(other.current) { + if (throw_on_copy) { + throw throwing_copy_error{}; + } + + ++live_count; + } + + throwing_forward_iterator(throwing_forward_iterator&& other) noexcept + : current(std::exchange(other.current, nullptr)) { + ++live_count; + } + + throwing_forward_iterator& operator=(const throwing_forward_iterator& other) { + if (throw_on_copy) { + throw throwing_copy_error{}; + } + + current = other.current; + return *this; + } + + throwing_forward_iterator& operator=(throwing_forward_iterator&& other) noexcept { + current = std::exchange(other.current, nullptr); + return *this; + } + + ~throwing_forward_iterator() { --live_count; } + + int& operator*() const noexcept { return *current; } + + throwing_forward_iterator& operator++() noexcept { + ++current; + return *this; + } + + throwing_forward_iterator operator++(int) { + auto previous = *this; + ++*this; + return previous; + } + + friend bool operator==(const throwing_forward_iterator&, const throwing_forward_iterator&) = default; +}; + +static_assert(std::forward_iterator); + +struct throwing_forward_view : std::ranges::view_base { + int* first{}; + int* last{}; + + throwing_forward_view() = default; + + throwing_forward_view(int* first, int* last) : first(first), last(last) {} + + throwing_forward_iterator begin() const noexcept { return throwing_forward_iterator{first}; } + + throwing_forward_iterator end() const noexcept { return throwing_forward_iterator{last}; } +}; + +static_assert(std::ranges::forward_range); diff --git a/tests/beman/any_view/regressions.test.cpp b/tests/beman/any_view/regressions.test.cpp new file mode 100644 index 0000000..c8843b7 --- /dev/null +++ b/tests/beman/any_view/regressions.test.cpp @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include "detail/self_ref_input_view.hpp" +#include "detail/throwing_forward_view.hpp" +#include "detail/self_ref_forward_proxy_view.hpp" + +#include + +#include + +using beman::any_view::any_view; +using enum beman::any_view::any_view_options; + +template +using any_random_access_view = any_view; + +// GH-85 +TEST(RegressionTest, moving_input_iterator_rebinds_dereference) { + any_view view{self_ref_input_view{42, 43}}; + + auto source = view.begin(); + auto moved = std::move(source); + + EXPECT_EQ(*moved, 42); +} + +// GH-85 +TEST(RegressionTest, move_assigning_input_iterator_rebinds_dereference) { + any_view view{self_ref_input_view{42, 43}}; + + auto destination = view.begin(); + ++destination; + + auto source = view.begin(); + destination = std::move(source); + + EXPECT_EQ(*destination, 42); +} + +using proxy_forward_any_view = any_view; + +// GH-85 +TEST(RegressionTest, copying_forward_proxy_iterator_preserves_position) { + proxy_forward_any_view view{self_ref_forward_proxy_view{42, 44}}; + + auto source = view.begin(); + auto copied = source; + + ++source; + + // The copied underlying iterator remains at 42. + // A copied cache incorrectly points into source and returns 43. + EXPECT_EQ(static_cast(*copied), 42); +} + +// GH-85 +TEST(RegressionTest, moving_forward_proxy_iterator_preserves_position) { + proxy_forward_any_view view{self_ref_forward_proxy_view{42, 44}}; + + auto source = view.begin(); + auto moved = std::move(source); + + // A stale cache points into moved-from source and returns -777. + EXPECT_EQ(static_cast(*moved), 42); +} + +// GH-85 +TEST(RegressionTest, copy_assigning_forward_proxy_iterator_preserves_position) { + proxy_forward_any_view view{self_ref_forward_proxy_view{42, 44}}; + + auto source = view.begin(); + auto destination = view.begin(); + ++destination; + + destination = source; + ++source; + + EXPECT_EQ(static_cast(*destination), 42); +} + +// GH-85 +TEST(RegressionTest, move_assigning_forward_proxy_iterator_preserves_position) { + proxy_forward_any_view view{self_ref_forward_proxy_view{42, 44}}; + + auto source = view.begin(); + auto destination = view.begin(); + ++destination; + + destination = std::move(source); + + EXPECT_EQ(static_cast(*destination), 42); +} + +// GH-86 +TEST(RegressionTest, copy_assigning_forward_iterator_is_exception_safe) { + EXPECT_EQ(throwing_forward_iterator::live_count, 0); + + { + int values[]{1, 2, 3}; + + any_view view{throwing_forward_view{values, values + 3}}; + + auto source = view.begin(); + auto destination = view.begin(); + ++destination; + + ASSERT_EQ(*source, 1); + ASSERT_EQ(*destination, 2); + + const int live_before = throwing_forward_iterator::live_count; + + throwing_forward_iterator::throw_on_copy = true; + + EXPECT_THROW(destination = source, throwing_copy_error); + + throwing_forward_iterator::throw_on_copy = false; + + // The failed replacement must not have destroyed the + // destination's erased iterator and sentinel. + EXPECT_EQ(throwing_forward_iterator::live_count, live_before); + + // Strong guarantee: destination retains its original value. + EXPECT_EQ(*destination, 2); + + // The source was not changed either. + EXPECT_EQ(*source, 1); + } + + EXPECT_EQ(throwing_forward_iterator::live_count, 0); +} + +TEST(RegressionTest, random_access_proxy_iterator_reaches_end) { + std::vector values{true, false, true}; + using proxy = std::ranges::range_reference_t; + + any_view view{values}; + + auto iterator = view.begin(); + ++iterator; + ++iterator; + ++iterator; + + EXPECT_EQ(iterator, view.end()); +}