From f010ebf8f1f730787ca464fe2ff9342b7336c9cd Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Mon, 17 Dec 2018 18:39:46 +0000 Subject: [PATCH 1/7] Allow `make_range` to construct a range of non-const values This is required in order to allow values in the constructed range to be moved from. --- src/util/range.h | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/util/range.h b/src/util/range.h index f994430b82d..210a26b1d04 100644 --- a/src/util/range.h +++ b/src/util/range.h @@ -357,12 +357,11 @@ ranget make_range(iteratort begin, iteratort end) return ranget(begin, end); } -template < - typename containert, - typename iteratort = typename containert::const_iterator> -ranget make_range(const containert &container) +template +auto make_range(containert &container) -> ranget { - return ranget(container.begin(), container.end()); + return ranget( + container.begin(), container.end()); } #endif // CPROVER_UTIL_RANGE_H From 181becfab367f88ca9814014a80c9ac7b20de601 Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Mon, 17 Dec 2018 18:39:47 +0000 Subject: [PATCH 2/7] Add non-const overloads of the `*` and `->` for `filter_iteratort` This adds support for moving from the values accessible through the result of `ranget.filter`. --- src/util/range.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/util/range.h b/src/util/range.h index 210a26b1d04..091d73b3459 100644 --- a/src/util/range.h +++ b/src/util/range.h @@ -142,6 +142,16 @@ class filter_iteratort return tmp; } + value_type &operator*() + { + return *underlying; + } + + value_type *operator->() + { + return &(*underlying); + } + const value_type &operator*() const { return *underlying; From 16bc92fe12965d099db4a222433bdc045711aadf Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Mon, 17 Dec 2018 18:39:48 +0000 Subject: [PATCH 3/7] Add non-const overloads of the `*` and `->` for `concat_iteratort` This adds support for moving from the values accessible through the result of `ranget.concat`. --- src/util/range.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/util/range.h b/src/util/range.h index 091d73b3459..c4e15a40fc0 100644 --- a/src/util/range.h +++ b/src/util/range.h @@ -244,6 +244,20 @@ struct concat_iteratort return tmp; } + value_type &operator*() + { + if(first_begin == first_end) + return *second_begin; + return *first_begin; + } + + value_type *operator->() + { + if(first_begin == first_end) + return &(*second_begin); + return &(*first_begin); + } + const value_type &operator*() const { if(first_begin == first_end) From 81e4a938cec08978ac8779be8f343c635fab42bb Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Mon, 17 Dec 2018 18:39:49 +0000 Subject: [PATCH 4/7] Update `map_iteratort` to use a `shared_ptr` for `current` This allows mapping into move only types. This was not previously possible, because the copy constructor of `map_iteratort` copied the value pointed to by its unique pointer in its `current` field. This can also be considered to be more correct, because copying an iterator should be expected to yield two copies, which point to the same value, rather than two copies, which point to two separate values. --- src/util/range.h | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/util/range.h b/src/util/range.h index c4e15a40fc0..8bdf986ec85 100644 --- a/src/util/range.h +++ b/src/util/range.h @@ -48,7 +48,7 @@ class map_iteratort PRECONDITION(underlying != underlying_end); ++underlying; if(underlying != underlying_end) - current = util_make_unique((*f)(*underlying)); + current = std::make_shared((*f)(*underlying)); return *this; } @@ -80,16 +80,7 @@ class map_iteratort f(std::move(f)) { if(this->underlying != this->underlying_end) - current = util_make_unique((*this->f)(*this->underlying)); - } - - map_iteratort(const map_iteratort &other) - : underlying(other.underlying), - underlying_end(other.underlying_end), - f(other.f) - { - if(other.current != nullptr) - current = util_make_unique(*other.current.get()); + current = std::make_shared((*this->f)(*this->underlying)); } private: @@ -101,7 +92,7 @@ class map_iteratort /// Stores the result of \c f at the current position of the iterator. /// Equals nullptr if the iterator reached \c underlying_end. - std::unique_ptr current = nullptr; + std::shared_ptr current = nullptr; }; /// Iterator which only stops at elements for which some given function \c f is From a6832e3ff106472d76d296e10021f08724fa7f68 Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Mon, 17 Dec 2018 18:39:51 +0000 Subject: [PATCH 5/7] Add non-const overloads of the `*` and `->` for `map_iteratort` This adds support for moving from the values accessible through the result of `ranget.map`. --- src/util/range.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/util/range.h b/src/util/range.h index 8bdf986ec85..280c689796c 100644 --- a/src/util/range.h +++ b/src/util/range.h @@ -60,6 +60,16 @@ class map_iteratort return tmp; } + value_type &operator*() + { + return *current.get(); + } + + value_type *operator->() + { + return &(*current.get()); + } + const value_type &operator*() const { return *current.get(); From 0b73746794271d3276a8ba0df02484f3d5657756 Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Mon, 17 Dec 2018 18:39:52 +0000 Subject: [PATCH 6/7] Add unit test on ranges of move only values --- unit/util/range.cpp | 118 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/unit/util/range.cpp b/unit/util/range.cpp index 7da825c42c0..156631bdfa8 100644 --- a/unit/util/range.cpp +++ b/unit/util/range.cpp @@ -65,3 +65,121 @@ SCENARIO("range tests", "[core][util][range]") } } } + +class move_onlyt +{ +public: + move_onlyt(move_onlyt &&) = default; + move_onlyt &operator=(move_onlyt &&) = default; + move_onlyt(const move_onlyt &) = delete; + move_onlyt &operator=(const move_onlyt &) = delete; + + explicit move_onlyt(int value) : value{value} {}; + int value = 0; +}; + +bool is_odd(const move_onlyt &move_only) +{ + return move_only.value % 2 != 0; +} + +const auto add = [](int left) { + return [=](const move_onlyt &right) { return left + right.value; }; +}; + +SCENARIO( + "Range tests, with collections of move only typed values.", + "[core][util][range]") +{ + GIVEN("A vector of move only typed values.") + { + std::vector input; + for(int i = 1; i <= 10; ++i) + input.emplace_back(i); + THEN("Values from a range of made from the vector can be moved.") + { + auto input_range = make_range(input); + move_onlyt destination{std::move(*input_range.begin())}; + REQUIRE(destination.value == 1); + } + THEN("A range of made from the vector can be filtered.") + { + auto odds_filter = make_range(input).filter(is_odd); + std::size_t total = 0; + for(const auto &move_only : odds_filter) + total += 1; + REQUIRE(total == 5); + auto iterator = odds_filter.begin(); + REQUIRE((iterator++)->value == 1); + REQUIRE((iterator++)->value == 3); + REQUIRE((iterator++)->value == 5); + REQUIRE((iterator++)->value == 7); + REQUIRE((iterator++)->value == 9); + } + THEN("Values from a filtered range made from the vector can be moved.") + { + std::vector odds; + for(move_onlyt &odd : make_range(input).filter(is_odd)) + odds.emplace_back(std::move(odd)); + + REQUIRE(odds.size() == 5); + REQUIRE(odds[0].value == 1); + REQUIRE(odds[1].value == 3); + REQUIRE(odds[2].value == 5); + REQUIRE(odds[3].value == 7); + REQUIRE(odds[4].value == 9); + } + THEN("Map can be applied to a range of move only typed values.") + { + std::vector results; + for(int result : make_range(input).map(add(1))) + results.push_back(result); + + const std::vector expected_results{2, 3, 4, 5, 6, 7, 8, 9, 10, 11}; + REQUIRE(results == expected_results); + } + } + GIVEN("Two vectors containing move only types values.") + { + std::vector input1; + for(int i = 1; i <= 3; ++i) + input1.emplace_back(i); + std::vector input2; + for(int i = 7; i <= 9; ++i) + input2.emplace_back(i); + + THEN("Values from concatenated ranges made from the vector can be moved.") + { + std::vector both_inputs; + for(move_onlyt &input : make_range(input1).concat(make_range(input2))) + both_inputs.emplace_back(std::move(input)); + + REQUIRE(both_inputs.size() == 6); + REQUIRE(both_inputs[0].value == 1); + REQUIRE(both_inputs[1].value == 2); + REQUIRE(both_inputs[2].value == 3); + REQUIRE(both_inputs[3].value == 7); + REQUIRE(both_inputs[4].value == 8); + REQUIRE(both_inputs[5].value == 9); + } + } + GIVEN("A const vector of ints.") + { + const std::vector input{1, 2, 3, 4, 5}; + + THEN("The vector can be mapped into a range of move-only types") + { + std::vector results; + const auto make_move_only = [](int i) { return move_onlyt{i}; }; + for(auto &incremented : make_range(input).map(make_move_only)) + results.emplace_back(std::move(incremented)); + + REQUIRE(results.size() == 5); + REQUIRE(results[0].value == 1); + REQUIRE(results[1].value == 2); + REQUIRE(results[2].value == 3); + REQUIRE(results[3].value == 4); + REQUIRE(results[4].value == 5); + } + } +} From bbc201668e32867361841f2ad4eba3ecafe3185b Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Tue, 18 Dec 2018 11:13:45 +0000 Subject: [PATCH 7/7] Expand doxygen on `ranget.map` The new doxygen explains the extent and limitations of `ranget.map`'s support of move-only types. --- src/util/range.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/util/range.h b/src/util/range.h index 280c689796c..72420c8404d 100644 --- a/src/util/range.h +++ b/src/util/range.h @@ -327,7 +327,11 @@ struct ranget final } /// The type of elements contained in the resulting range is deduced from the - /// return type of \p `f`. + /// return type of `f`. + /// Please note that the parameter to `f` must be a const reference. This is + /// a limitation of the current implementation. This means that you can't move + /// a value through `f`. `f` may take a move-only typed parameter by const + /// reference. 'f' may also construct and return a move-only typed value. template auto map(functiont &&f) -> ranget