From b23af215e8ab0a59a444a521c2030b1905604158 Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Fri, 23 Aug 2024 22:13:34 +0100 Subject: [PATCH 01/10] Handle empty range insertion --- src/include/interval-map/interval-map.hh | 2 ++ tests/unit/unit_test.cc | 26 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/src/include/interval-map/interval-map.hh b/src/include/interval-map/interval-map.hh index ce1d9dd..8f806c3 100644 --- a/src/include/interval-map/interval-map.hh +++ b/src/include/interval-map/interval-map.hh @@ -9,6 +9,8 @@ public: interval_map(V const& init) : init_(init) {} void assign(K const& begin, K const& end, V const& val) { + if (!(begin < end)) + return; // TODO: implement } diff --git a/tests/unit/unit_test.cc b/tests/unit/unit_test.cc index 288c811..b2b990e 100644 --- a/tests/unit/unit_test.cc +++ b/tests/unit/unit_test.cc @@ -66,3 +66,29 @@ TEST(interval_map, minimal_interface) { ASSERT_EQ(map[Key(0)], Value(0)); map.assign(Key(0), Key(1), Value(1)); } + +TEST(interval_map, no_insertion) { + auto map = amby::interval_map{0}; + for (int i = std::numeric_limits::min(); + i <= std::numeric_limits::max(); ++i) { + ASSERT_EQ(map[i], 0); + } +} + +TEST(interval_map, insert_begin_equal_end) { + auto map = amby::interval_map{0}; + map.assign(0, 0, 1); + for (int i = std::numeric_limits::min(); + i <= std::numeric_limits::max(); ++i) { + ASSERT_EQ(map[i], 0); + } +} + +TEST(interval_map, insert_begin_bigger_than_end) { + auto map = amby::interval_map{0}; + map.assign(1, 0, 1); + for (int i = std::numeric_limits::min(); + i <= std::numeric_limits::max(); ++i) { + ASSERT_EQ(map[i], 0); + } +} From 5868b5d36cd358b7914efb48fb4398dba85178e9 Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Fri, 23 Aug 2024 23:15:12 +0100 Subject: [PATCH 02/10] Introduce 'IntervalMapTest' fixture --- tests/unit/unit_test.cc | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/tests/unit/unit_test.cc b/tests/unit/unit_test.cc index b2b990e..3cdf79e 100644 --- a/tests/unit/unit_test.cc +++ b/tests/unit/unit_test.cc @@ -58,7 +58,20 @@ static_assert(std::is_same_v< ValueInterface, decltype(std::numeric_limits>::lowest())>); -TEST(interval_map, minimal_interface) { +class IntervalMapTest : public testing::Test { +protected: + using key_type = char; + using value_type = int; + using map_type = amby::interval_map; + + map_type map{0}; + + void SetUp() override { + map = map_type{0}; + } +}; + +TEST_F(IntervalMapTest, minimal_interface) { using Key = KeyInterface; using Value = ValueInterface; @@ -67,16 +80,14 @@ TEST(interval_map, minimal_interface) { map.assign(Key(0), Key(1), Value(1)); } -TEST(interval_map, no_insertion) { - auto map = amby::interval_map{0}; +TEST_F(IntervalMapTest, no_insertion) { for (int i = std::numeric_limits::min(); i <= std::numeric_limits::max(); ++i) { ASSERT_EQ(map[i], 0); } } -TEST(interval_map, insert_begin_equal_end) { - auto map = amby::interval_map{0}; +TEST_F(IntervalMapTest, insert_begin_equal_end) { map.assign(0, 0, 1); for (int i = std::numeric_limits::min(); i <= std::numeric_limits::max(); ++i) { @@ -84,8 +95,7 @@ TEST(interval_map, insert_begin_equal_end) { } } -TEST(interval_map, insert_begin_bigger_than_end) { - auto map = amby::interval_map{0}; +TEST_F(IntervalMapTest, insert_begin_bigger_than_end) { map.assign(1, 0, 1); for (int i = std::numeric_limits::min(); i <= std::numeric_limits::max(); ++i) { From ce7cc4492ce867e8632f55a930d123e937d53af1 Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Fri, 23 Aug 2024 23:16:39 +0100 Subject: [PATCH 03/10] Explicitly test for canonicity This will come in handy later, with more complex test cases. --- src/include/interval-map/interval-map.hh | 6 ++++++ tests/unit/unit_test.cc | 21 +++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/include/interval-map/interval-map.hh b/src/include/interval-map/interval-map.hh index 8f806c3..4dc2335 100644 --- a/src/include/interval-map/interval-map.hh +++ b/src/include/interval-map/interval-map.hh @@ -2,6 +2,9 @@ #include +// Testing class forward declaration +class IntervalMapTest; + namespace amby { template class interval_map { @@ -21,6 +24,9 @@ public: return std::prev(it)->second; } + // Used in testing + friend class ::IntervalMapTest; + private: V init_; std::map underlying_{}; diff --git a/tests/unit/unit_test.cc b/tests/unit/unit_test.cc index 3cdf79e..d51db5a 100644 --- a/tests/unit/unit_test.cc +++ b/tests/unit/unit_test.cc @@ -69,6 +69,27 @@ protected: void SetUp() override { map = map_type{0}; } + + void TearDown() override { + check_canonicity(); + } + + void check_canonicity() const { + // Consecutive map entries must not contain the same value + for (auto it = map.underlying_.begin(); it != map.underlying_.end(); + ++it) { + const auto next = std::next(it, 1); + if (next == map.underlying_.end()) + break; + EXPECT_NE(it->second, next->second); + } + + // The first entry must not contain the initial value + if (const auto it = map.underlying_.begin(); + it != map.underlying_.end()) { + EXPECT_NE(it->second, map.init_); + } + } }; TEST_F(IntervalMapTest, minimal_interface) { From 1126ee30d8e762a79b4575988d2e1ee3552bba6c Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Fri, 23 Aug 2024 23:38:05 +0100 Subject: [PATCH 04/10] Test against fake 'Model' implementation This removes most of the redundant `for` loops that would appear in hand-written unit tests, and could potentially open the door to fuzz-testing/property-test the implementation for a more in-depth test-suite. --- tests/unit/model.hh | 35 ++++++++++++++++++++++++++++++++ tests/unit/unit_test.cc | 44 ++++++++++++++++++++++++++--------------- 2 files changed, 63 insertions(+), 16 deletions(-) create mode 100644 tests/unit/model.hh diff --git a/tests/unit/model.hh b/tests/unit/model.hh new file mode 100644 index 0000000..9acb19f --- /dev/null +++ b/tests/unit/model.hh @@ -0,0 +1,35 @@ +#pragma once + +#include + +template struct Range { + K begin; + K end; + V val; +}; + +// Same behaviour as interval_map, but implementation is trivally correct +template class Model { +public: + Model(V const& init) : init_(init) {} + + void assign(K const& begin, K const& end, V const& val) { + if (!(begin < end)) + return; + ranges_.emplace_back(begin, end, val); + } + + V const& operator[](K const& key) const { + for (auto it = ranges_.rbegin(); it != ranges_.rend(); ++it) { + if (key < it->begin) + continue; + if (it->end <= key) + continue; + return it->val; + } + return init_; + } + + V init_; + std::vector> ranges_{}; +}; diff --git a/tests/unit/unit_test.cc b/tests/unit/unit_test.cc index d51db5a..7b9e4ce 100644 --- a/tests/unit/unit_test.cc +++ b/tests/unit/unit_test.cc @@ -4,6 +4,8 @@ #include +#include "model.hh" + template class KeyInterface { public: explicit KeyInterface(T val) : underlying_(val) {} @@ -63,17 +65,40 @@ protected: using key_type = char; using value_type = int; using map_type = amby::interval_map; + using model_type = Model; map_type map{0}; + model_type model{0}; void SetUp() override { map = map_type{0}; + model = model_type{0}; } void TearDown() override { + check(); + } + + void assign(key_type const& begin, key_type const& end, + value_type const& val) { + map.assign(begin, end, val); + model.assign(begin, end, val); + } + + void check() const { + check_ranges(); check_canonicity(); } + // Compare against the fake 'Model' implementation + void check_ranges() const { + auto i = std::numeric_limits::min(); + for (; i < std::numeric_limits::max(); ++i) { + ASSERT_EQ(map[i], model[i]) << "(i: " << +i << ")"; + } + ASSERT_EQ(map[i], model[i]) << "(i: " << +i << ")"; + }; + void check_canonicity() const { // Consecutive map entries must not contain the same value for (auto it = map.underlying_.begin(); it != map.underlying_.end(); @@ -101,25 +126,12 @@ TEST_F(IntervalMapTest, minimal_interface) { map.assign(Key(0), Key(1), Value(1)); } -TEST_F(IntervalMapTest, no_insertion) { - for (int i = std::numeric_limits::min(); - i <= std::numeric_limits::max(); ++i) { - ASSERT_EQ(map[i], 0); - } -} +TEST_F(IntervalMapTest, no_insertion) {} TEST_F(IntervalMapTest, insert_begin_equal_end) { - map.assign(0, 0, 1); - for (int i = std::numeric_limits::min(); - i <= std::numeric_limits::max(); ++i) { - ASSERT_EQ(map[i], 0); - } + assign(0, 0, 1); } TEST_F(IntervalMapTest, insert_begin_bigger_than_end) { - map.assign(1, 0, 1); - for (int i = std::numeric_limits::min(); - i <= std::numeric_limits::max(); ++i) { - ASSERT_EQ(map[i], 0); - } + assign(1, 0, 1); } From 9d9091630712bb360d788dbb5449c002bb1e7025 Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Fri, 23 Aug 2024 23:49:52 +0100 Subject: [PATCH 05/10] Add implementation --- src/include/interval-map/interval-map.hh | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/include/interval-map/interval-map.hh b/src/include/interval-map/interval-map.hh index 4dc2335..1352b79 100644 --- a/src/include/interval-map/interval-map.hh +++ b/src/include/interval-map/interval-map.hh @@ -14,7 +14,16 @@ public: void assign(K const& begin, K const& end, V const& val) { if (!(begin < end)) return; - // TODO: implement + + auto const end_val = (*this)[end]; + underlying_.erase(underlying_.lower_bound(begin), + underlying_.upper_bound(end)); + if (!((*this)[begin] == val)) { + underlying_.insert({begin, val}); + } + if (!((*this)[end] == end_val)) { + underlying_.insert({end, end_val}); + } } V const& operator[](K const& key) const { From 301945884ab80083cab1090a9ece4b167020dcdf Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Sat, 24 Aug 2024 16:48:30 +0100 Subject: [PATCH 06/10] Add initial tests Those were the ones I used for the initial implementation. --- tests/unit/unit_test.cc | 67 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/tests/unit/unit_test.cc b/tests/unit/unit_test.cc index 7b9e4ce..a99c643 100644 --- a/tests/unit/unit_test.cc +++ b/tests/unit/unit_test.cc @@ -135,3 +135,70 @@ TEST_F(IntervalMapTest, insert_begin_equal_end) { TEST_F(IntervalMapTest, insert_begin_bigger_than_end) { assign(1, 0, 1); } + +TEST_F(IntervalMapTest, insert_one_range) { + assign(std::numeric_limits::min(), 0, 1); +} + +TEST_F(IntervalMapTest, insert_non_overlapping_ranges) { + assign(std::numeric_limits::min(), 0, 1); + assign(10, std::numeric_limits::max(), 2); +} + +TEST_F(IntervalMapTest, insert_up_to_max) { + assign(std::numeric_limits::min(), + std::numeric_limits::max(), 1); +} + +TEST_F(IntervalMapTest, insert_range_right_after) { + assign(0, 10, 1); + assign(10, 20, 1); +} + +TEST_F(IntervalMapTest, insert_range_right_before) { + assign(10, 20, 1); + assign(0, 10, 1); +} + +TEST_F(IntervalMapTest, insert_range_middle) { + assign(0, 10, 1); + assign(20, 30, 1); + assign(10, 20, 1); +} + +TEST_F(IntervalMapTest, insert_range_inside_another) { + assign(0, 20, 1); + assign(5, 15, 2); +} + +TEST_F(IntervalMapTest, insert_range_around_another) { + assign(5, 15, 2); + assign(0, 20, 1); +} + +TEST_F(IntervalMapTest, insert_range_overlaps_many) { + assign(0, 10, 1); + assign(10, 20, 2); + assign(20, 30, 3); + assign(30, 40, 4); + assign(40, 50, 5); + assign(0, 50, -1); +} + +TEST_F(IntervalMapTest, insert_range_overlaps_many_init_value) { + assign(0, 10, 1); + assign(10, 20, 2); + assign(20, 30, 3); + assign(30, 40, 4); + assign(40, 50, 5); + assign(0, 50, 0); +} + +TEST_F(IntervalMapTest, insert_range_overlaps_many_oversize) { + assign(0, 10, 1); + assign(10, 20, 2); + assign(20, 30, 3); + assign(30, 40, 4); + assign(40, 50, 5); + assign(-10, 60, -1); +} From 67ed4d4d68e466b2fae59c1d0c362d463f0579e2 Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Sat, 24 Aug 2024 19:19:27 +0100 Subject: [PATCH 07/10] Make test failure more verbose Outputting the state of the map and this history of operations makes debugging easier. --- tests/unit/unit_test.cc | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/unit/unit_test.cc b/tests/unit/unit_test.cc index a99c643..a5d2c5c 100644 --- a/tests/unit/unit_test.cc +++ b/tests/unit/unit_test.cc @@ -2,6 +2,7 @@ #include +#include #include #include "model.hh" @@ -86,10 +87,28 @@ protected: } void check() const { + SCOPED_TRACE(stringify_map()); + SCOPED_TRACE(stringify_operations()); check_ranges(); check_canonicity(); } + std::string stringify_map() const { + std::ostringstream out; + out << "map: "; + for (const auto& [key, val] : map.underlying_) + out << "[" << +key << ": " << +val << "]"; + return out.str(); + } + + std::string stringify_operations() const { + std::ostringstream out; + out << "ops: "; + for (const auto& [start, end, val] : model.ranges_) + out << "[" << +start << ":" << +end << " => " << +val << "]"; + return out.str(); + } + // Compare against the fake 'Model' implementation void check_ranges() const { auto i = std::numeric_limits::min(); From 8165fc6f99deef2f905ec70b8cc48c06e87aeed0 Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Sat, 24 Aug 2024 16:48:30 +0100 Subject: [PATCH 08/10] Add randomized test This was especially helpful for my previous attempt at a solution, which did was more complicated. The original rules for this assignment are quite silly, I don't think they really optimize anything, and make understanding the actual algorithm more difficult than it should be. --- tests/unit/unit_test.cc | 42 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/unit/unit_test.cc b/tests/unit/unit_test.cc index a5d2c5c..c429d29 100644 --- a/tests/unit/unit_test.cc +++ b/tests/unit/unit_test.cc @@ -2,6 +2,7 @@ #include +#include #include #include @@ -221,3 +222,44 @@ TEST_F(IntervalMapTest, insert_range_overlaps_many_oversize) { assign(40, 50, 5); assign(-10, 60, -1); } + +TEST_F(IntervalMapTest, fuzzing_001) { + assign(-50, 20, 1); + assign(40, 80, 2); + assign(-100, -10, 3); +} + +TEST_F(IntervalMapTest, fuzzing_002) { + assign(-100, 90, 1); + assign(0, 120, 2); + assign(-60, 60, 3); +} + +TEST_F(IntervalMapTest, fuzzing_003) { + assign(-80, 70, 1); + assign(-50, 40, 2); + assign(-40, 20, 3); + assign(-110, -10, 4); +} + +TEST_F(IntervalMapTest, randomized_test) { + auto const seed = []() { + std::random_device r; + return r(); + }(); + SCOPED_TRACE(seed); + auto random = std::mt19937_64(seed); + + auto keys = std::uniform_int_distribution( + std::numeric_limits::min(), + std::numeric_limits::max()); + auto values = std::uniform_int_distribution(0, 10); + + for (auto i = 0; i < 1000; ++i) { + auto const start = keys(random); + auto const end = keys(random); + auto const value = values(random); + assign(start, end, value); + check(); + } +} From bdccd17936479d4939738d1d9373e3cddede9c40 Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Sat, 24 Aug 2024 19:29:09 +0100 Subject: [PATCH 09/10] fixup! Add implementation --- src/include/interval-map/interval-map.hh | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/include/interval-map/interval-map.hh b/src/include/interval-map/interval-map.hh index 1352b79..a514daf 100644 --- a/src/include/interval-map/interval-map.hh +++ b/src/include/interval-map/interval-map.hh @@ -18,12 +18,10 @@ public: auto const end_val = (*this)[end]; underlying_.erase(underlying_.lower_bound(begin), underlying_.upper_bound(end)); - if (!((*this)[begin] == val)) { + if (!((*this)[begin] == val)) underlying_.insert({begin, val}); - } - if (!((*this)[end] == end_val)) { + if (!((*this)[end] == end_val)) underlying_.insert({end, end_val}); - } } V const& operator[](K const& key) const { From dc3408524b0bf7bf4adf0300c6067126dc9e1048 Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Sat, 24 Aug 2024 20:19:32 +0100 Subject: [PATCH 10/10] Make implementation follow assignment rules The assignment wants us to use exactly *one* O(log(N)) call. Hence the single usage of `upper_bound`. It also wants us to use a minimal amount of operations on `K` and `V` values in the map. Finally it asks us to make the answer simple. --- src/include/interval-map/interval-map.hh | 41 +++++++++++++++++------- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/src/include/interval-map/interval-map.hh b/src/include/interval-map/interval-map.hh index a514daf..ab3fb44 100644 --- a/src/include/interval-map/interval-map.hh +++ b/src/include/interval-map/interval-map.hh @@ -15,26 +15,45 @@ public: if (!(begin < end)) return; - auto const end_val = (*this)[end]; - underlying_.erase(underlying_.lower_bound(begin), - underlying_.upper_bound(end)); - if (!((*this)[begin] == val)) - underlying_.insert({begin, val}); - if (!((*this)[end] == end_val)) - underlying_.insert({end, end_val}); + auto it = underlying_.upper_bound(end); + auto const end_val = at_upper_bound(it); + + bool insert_begin = !(val == init_); + + while (it != underlying_.begin()) { + it = std::prev(it); + auto begin_found = it->first < begin; + if (begin_found) { + insert_begin = !(val == it->second); + break; + } + if (it != underlying_.end()) + // Account for up-coming `std::prev` with `++` + underlying_.erase(it++); + } + + if (insert_begin) + it = underlying_.insert(it, {begin, val}); + // Get the proper upper-bound for `end` + it = (it == underlying_.end()) ? it : std::next(it); + if (!(at_upper_bound(it) == end_val)) + underlying_.insert(it, {end, end_val}); } V const& operator[](K const& key) const { - auto it = underlying_.upper_bound(key); - if (it == underlying_.begin()) - return init_; - return std::prev(it)->second; + return at_upper_bound(underlying_.upper_bound(key)); } // Used in testing friend class ::IntervalMapTest; private: + V const& at_upper_bound(std::map::const_iterator it) const { + if (it == underlying_.begin()) + return init_; + return std::prev(it)->second; + } + V init_; std::map underlying_{}; };