From 4bd2317eb56c09510adbd30187d21e4284d36033 Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Fri, 18 Jan 2019 16:35:21 +0000 Subject: [PATCH 1/6] Fix constness on dereferencing a `filter_iteratort` Dereferencing a `filter_iteratort` from filtering a const collection with `ranget` would previously introduce a compile error, due to a `const` to non-const conversion. This commit fixes this, by using the same return type as the iterator which it wraps around. This commit includes a test for using range to filter a `const` collection. --- src/util/range.h | 8 ++++---- unit/util/range.cpp | 12 ++++++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/util/range.h b/src/util/range.h index 72420c8404d..ea8f47b02d5 100644 --- a/src/util/range.h +++ b/src/util/range.h @@ -113,8 +113,8 @@ class filter_iteratort public: using difference_type = typename iteratort::difference_type; using value_type = typename iteratort::value_type; - using pointer = const value_type *; - using reference = const value_type &; + using pointer = typename iteratort::pointer; + using reference = typename iteratort::reference; using iterator_category = std::forward_iterator_tag; bool operator==(const filter_iteratort &other) const @@ -143,12 +143,12 @@ class filter_iteratort return tmp; } - value_type &operator*() + reference operator*() { return *underlying; } - value_type *operator->() + pointer operator->() { return &(*underlying); } diff --git a/unit/util/range.cpp b/unit/util/range.cpp index 4ab3c6f5a70..1b83c11e487 100644 --- a/unit/util/range.cpp +++ b/unit/util/range.cpp @@ -64,6 +64,18 @@ SCENARIO("range tests", "[core][util][range]") REQUIRE(total == 8); } } + GIVEN("A const vector of ints") + { + const std::vector input{1, 2, 3, 4}; + THEN("Filter the vector using range.") + { + auto odds_range = + make_range(input).filter([](const int number) { return number % 2; }); + const std::vector odds{odds_range.begin(), odds_range.end()}; + const std::vector expected_odds{1, 3}; + REQUIRE(odds == expected_odds); + } + } } class move_onlyt From f8738c99434e7f3516073e5f12a7654f0ff8dda1 Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Fri, 18 Jan 2019 16:35:23 +0000 Subject: [PATCH 2/6] Fix constness on dereferencing a `concat_iteratort` Dereferencing a `concat_iteratort` from concating const collections with `ranget` would previously introduce a compile error, due to a `const` to non-const conversion. This commit fixes this, by using the same return type as the iterator which it wraps around. This commit includes a test for using range to concat `const` collections. --- src/util/range.h | 8 ++++---- unit/util/range.cpp | 12 ++++++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/util/range.h b/src/util/range.h index ea8f47b02d5..b7de67dfd6e 100644 --- a/src/util/range.h +++ b/src/util/range.h @@ -208,8 +208,8 @@ struct concat_iteratort public: using difference_type = typename first_iteratort::difference_type; using value_type = typename first_iteratort::value_type; - using pointer = const value_type *; - using reference = const value_type &; + using pointer = typename first_iteratort::pointer; + using reference = typename first_iteratort::reference; using iterator_category = std::forward_iterator_tag; static_assert( @@ -245,14 +245,14 @@ struct concat_iteratort return tmp; } - value_type &operator*() + reference operator*() { if(first_begin == first_end) return *second_begin; return *first_begin; } - value_type *operator->() + pointer operator->() { if(first_begin == first_end) return &(*second_begin); diff --git a/unit/util/range.cpp b/unit/util/range.cpp index 1b83c11e487..1cf8f95bc17 100644 --- a/unit/util/range.cpp +++ b/unit/util/range.cpp @@ -76,6 +76,18 @@ SCENARIO("range tests", "[core][util][range]") REQUIRE(odds == expected_odds); } } + GIVEN("Two const vectors of ints") + { + const std::vector input1{1, 2}; + const std::vector input2{3, 4}; + THEN("Concat the vectors using range.") + { + auto range = make_range(input1).concat(make_range(input2)); + const std::vector output{range.begin(), range.end()}; + const std::vector expected{1, 2, 3, 4}; + REQUIRE(output == expected); + }; + } } class move_onlyt From 6060a878b2ab8fa9c258ee60a84aba584e3edb4d Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Fri, 18 Jan 2019 16:35:25 +0000 Subject: [PATCH 3/6] Add `const` to `begin()` accessor of `ranget` It was previously impossible to iterate over a `const` range, because you couldn't call `.begin()` on it. This fixes that issue. Changing this to `const` does not allow the internals of the range to be mutated, as it returns a copy of the begin iterator, not a reference to it. This commit adds `const` to ranges in range unit test to show that ranges can be made const correct, using this fix. --- src/util/range.h | 2 +- unit/util/range.cpp | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/util/range.h b/src/util/range.h index b7de67dfd6e..ce074f8e1e1 100644 --- a/src/util/range.h +++ b/src/util/range.h @@ -365,7 +365,7 @@ struct ranget final return begin_value == end_value; } - iteratort begin() + iteratort begin() const { return begin_value; } diff --git a/unit/util/range.cpp b/unit/util/range.cpp index 1cf8f95bc17..d3ae5684601 100644 --- a/unit/util/range.cpp +++ b/unit/util/range.cpp @@ -21,7 +21,7 @@ SCENARIO("range tests", "[core][util][range]") list.emplace_back("acdef"); THEN("Use range-for to compute the total length") { - auto range = make_range(list); + const auto range = make_range(list); std::size_t total_length = 0; for(const auto &s : range) total_length += s.length(); @@ -29,7 +29,7 @@ SCENARIO("range tests", "[core][util][range]") } THEN("Use map to compute individual lengths") { - auto length_range = + const auto length_range = make_range(list).map([](const std::string &s) { return s.length(); }); auto it = length_range.begin(); REQUIRE(*it == 3); @@ -42,7 +42,7 @@ SCENARIO("range tests", "[core][util][range]") } THEN("Filter using lengths") { - auto filtered_range = make_range(list).filter( + const auto filtered_range = make_range(list).filter( [&](const std::string &s) { return s.length() == 4; }); auto it = filtered_range.begin(); REQUIRE(*it == "cdef"); @@ -51,7 +51,7 @@ SCENARIO("range tests", "[core][util][range]") } THEN("Filter, map and use range-for on the same list") { - auto range = + const auto range = make_range(list) .filter([&](const std::string &s) -> bool { return s[0] == 'a'; }) .map([&](const std::string &s) { return s.length(); }); @@ -69,7 +69,7 @@ SCENARIO("range tests", "[core][util][range]") const std::vector input{1, 2, 3, 4}; THEN("Filter the vector using range.") { - auto odds_range = + const auto odds_range = make_range(input).filter([](const int number) { return number % 2; }); const std::vector odds{odds_range.begin(), odds_range.end()}; const std::vector expected_odds{1, 3}; @@ -82,7 +82,7 @@ SCENARIO("range tests", "[core][util][range]") const std::vector input2{3, 4}; THEN("Concat the vectors using range.") { - auto range = make_range(input1).concat(make_range(input2)); + const auto range = make_range(input1).concat(make_range(input2)); const std::vector output{range.begin(), range.end()}; const std::vector expected{1, 2, 3, 4}; REQUIRE(output == expected); @@ -122,13 +122,13 @@ SCENARIO( input.emplace_back(i); THEN("Values from a range of made from the vector can be moved.") { - auto input_range = make_range(input); + const 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); + const auto odds_filter = make_range(input).filter(is_odd); const std::size_t total = std::distance(odds_filter.begin(), odds_filter.end()); REQUIRE(total == 5); From 256fd817d502757b45829069e912e2f4281a6491 Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Fri, 18 Jan 2019 16:35:28 +0000 Subject: [PATCH 4/6] Rename `ranget::value_typet` to `ranget::value_type` Algorithms which are written expecting a STL container may reference a `value_type` member, as this is the name of the member in STL containers. Therefore this needs to be named the same way in `ranget` in order to be compatible with such algorithms. This commit includes a unit test of `ranget` having correct the `value_type` defined. --- src/util/range.h | 8 ++++---- unit/util/range.cpp | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/util/range.h b/src/util/range.h index ce074f8e1e1..de7198d7208 100644 --- a/src/util/range.h +++ b/src/util/range.h @@ -311,14 +311,14 @@ template struct ranget final { public: - using value_typet = typename iteratort::value_type; + using value_type = typename iteratort::value_type; ranget(iteratort begin, iteratort end) : begin_value(begin), end_value(end) { } ranget> - filter(std::function f) + filter(std::function f) { auto shared_f = std::make_shared(std::move(f)); filter_iteratort filter_begin(shared_f, begin(), end()); @@ -335,9 +335,9 @@ struct ranget final template auto map(functiont &&f) -> ranget::type>> + typename std::result_of::type>> { - using outputt = typename std::result_of::type; + using outputt = typename std::result_of::type; auto shared_f = std::make_shared< std::function>( std::forward(f)); diff --git a/unit/util/range.cpp b/unit/util/range.cpp index d3ae5684601..d306d479957 100644 --- a/unit/util/range.cpp +++ b/unit/util/range.cpp @@ -11,6 +11,14 @@ Author: Romain Brenguier, romain.brenguier@diffblue.com #include #include +/// Trivial example template function requiring a container to have a +/// `value_type`. +template +typename containert::value_type front(containert container) +{ + return *container.begin(); +} + SCENARIO("range tests", "[core][util][range]") { GIVEN("A vector with three strings") @@ -75,6 +83,18 @@ SCENARIO("range tests", "[core][util][range]") const std::vector expected_odds{1, 3}; REQUIRE(odds == expected_odds); } + THEN( + "The unit testing template function requiring `value_type` works with " + "`std::vector`.") + { + REQUIRE(front(input) == 1); + } + THEN( + "A range can be used with a template function expecting a container " + "which has a `value_type`.") + { + REQUIRE(front(make_range(input)) == 1); + } } GIVEN("Two const vectors of ints") { From 3da70826e892362ec70c4271c57fea62c7ec2533 Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Fri, 18 Jan 2019 16:35:30 +0000 Subject: [PATCH 5/6] Add unit test on range.map over a const container Given that `filter` and `concat` didn't work with const input collections, it seems prudent to test that `map` works with const input collections as well. There is no corresponding fix to make this test pass in this PR. --- unit/util/range.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/unit/util/range.cpp b/unit/util/range.cpp index d306d479957..665d589c04a 100644 --- a/unit/util/range.cpp +++ b/unit/util/range.cpp @@ -95,6 +95,15 @@ SCENARIO("range tests", "[core][util][range]") { REQUIRE(front(make_range(input)) == 1); } + THEN("Map over the vector using range.") + { + const auto plus_one_range = + make_range(input).map([](const int number) { return number + 1; }); + const std::vector plus_one_collection{plus_one_range.begin(), + plus_one_range.end()}; + const std::vector expected_output{2, 3, 4, 5}; + REQUIRE(plus_one_collection == expected_output); + }; } GIVEN("Two const vectors of ints") { From a89ca18e34f3098ceebb2756122454994306564b Mon Sep 17 00:00:00 2001 From: Thomas Spriggs Date: Fri, 18 Jan 2019 17:53:01 +0000 Subject: [PATCH 6/6] Provide `const` dereference of filter / concat iterators only Deferencing a `const` iterator which refers to a non-const value should enable mutation of that value. This is consistent with how iterators in the STL work. It means that we only need to provide a single const overload of these operators, not both the const and non-const variations. So the benefit of this commit is better consistency with the STL and fewer operator overloads to maintain. --- src/util/range.h | 32 ++++---------------------------- unit/util/range.cpp | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 28 deletions(-) diff --git a/src/util/range.h b/src/util/range.h index de7198d7208..0f91cbdabc1 100644 --- a/src/util/range.h +++ b/src/util/range.h @@ -143,22 +143,12 @@ class filter_iteratort return tmp; } - reference operator*() + reference operator*() const { return *underlying; } - pointer operator->() - { - return &(*underlying); - } - - const value_type &operator*() const - { - return *underlying; - } - - const value_type *operator->() const + pointer operator->() const { return &(*underlying); } @@ -245,28 +235,14 @@ struct concat_iteratort return tmp; } - reference operator*() - { - if(first_begin == first_end) - return *second_begin; - return *first_begin; - } - - pointer operator->() - { - if(first_begin == first_end) - return &(*second_begin); - return &(*first_begin); - } - - const value_type &operator*() const + reference operator*() const { if(first_begin == first_end) return *second_begin; return *first_begin; } - const value_type *operator->() const + pointer operator->() const { if(first_begin == first_end) return &(*second_begin); diff --git a/unit/util/range.cpp b/unit/util/range.cpp index 665d589c04a..f8bc6d7c299 100644 --- a/unit/util/range.cpp +++ b/unit/util/range.cpp @@ -57,6 +57,17 @@ SCENARIO("range tests", "[core][util][range]") ++it; REQUIRE(it == filtered_range.end()); } + THEN( + "A const instance of a `filter_iteratort` can mutate the input " + "collection.") + { + const auto it = + make_range(list) + .filter([&](const std::string &s) { return s.length() == 3; }) + .begin(); + *it += "x"; + REQUIRE(*list.begin() == "abcx"); + } THEN("Filter, map and use range-for on the same list") { const auto range = @@ -117,6 +128,27 @@ SCENARIO("range tests", "[core][util][range]") REQUIRE(output == expected); }; } + GIVEN("Two non-const vectors of ints.") + { + std::vector input1{1, 2}; + std::vector input2{3, 4}; + THEN( + "Const instances of `concat_iteratort` should enable the input " + "collections to be mutated.") + { + const auto concat_range = make_range(input1).concat(make_range(input2)); + int x = 5; + for(auto it = concat_range.begin(); it != concat_range.end(); ++it, ++x) + { + const auto const_it = it; + *const_it = x; + } + std::vector expected_result1{5, 6}; + std::vector expected_result2{7, 8}; + REQUIRE(input1 == expected_result1); + REQUIRE(input2 == expected_result2); + } + } } class move_onlyt