From 2a6b89e420765a761b5485487b33128e03e53c00 Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Sat, 24 Aug 2024 20:19:32 +0100 Subject: [PATCH 1/6] Make implementation follow assignment rules From what I could find, the rules of the assignment seem to be: 1. Restrict oneself to at most *one* O(log(N)) call, and otherwise use constant time operations on the map. 2. Don't use more operations than strictly necessary on `K` and `V`. 3. Prefer simplicity to performance. I think my solution would fair well under those constraints. --- 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_{}; }; From 8ea4fd373be5e4ee329c1732970c2f7fc8e1ae89 Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Sat, 24 Aug 2024 20:37:27 +0100 Subject: [PATCH 2/6] Fix Meson install --- src/meson.build | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/meson.build b/src/meson.build index 0e8c25d..3c30a37 100644 --- a/src/meson.build +++ b/src/meson.build @@ -1 +1,3 @@ interval_map_dep = declare_dependency(include_directories: 'include') + +install_headers('include/interval-map/interval-map.hh', subdir: 'interval-map') From 2317285d0fd70b1f794e7773190b9b535183fb26 Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Sat, 24 Aug 2024 20:59:14 +0100 Subject: [PATCH 3/6] Fix edge-case in identify 'end' upper-bound Just my luck, this was found immediately in the CI, right when I uploaded the project thinking I was done. Thankfully the fix is easy and (in hindsight) quite obvious. --- src/include/interval-map/interval-map.hh | 2 +- tests/unit/unit_test.cc | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/include/interval-map/interval-map.hh b/src/include/interval-map/interval-map.hh index ab3fb44..b518f24 100644 --- a/src/include/interval-map/interval-map.hh +++ b/src/include/interval-map/interval-map.hh @@ -35,7 +35,7 @@ public: if (insert_begin) it = underlying_.insert(it, {begin, val}); // Get the proper upper-bound for `end` - it = (it == underlying_.end()) ? it : std::next(it); + it = (it == underlying_.end() || end < it->first) ? it : std::next(it); if (!(at_upper_bound(it) == end_val)) underlying_.insert(it, {end, end_val}); } diff --git a/tests/unit/unit_test.cc b/tests/unit/unit_test.cc index c429d29..53675c3 100644 --- a/tests/unit/unit_test.cc +++ b/tests/unit/unit_test.cc @@ -242,6 +242,12 @@ TEST_F(IntervalMapTest, fuzzing_003) { assign(-110, -10, 4); } +TEST_F(IntervalMapTest, fuzzing_004) { + assign(-20, 120, 1); + assign(50, 110, 0); + assign(-120, 100, 0); +} + TEST_F(IntervalMapTest, randomized_test) { auto const seed = []() { std::random_device r; From 08f2364c50f22773284c52a2a19a8737d59d9539 Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Sat, 24 Aug 2024 20:19:32 +0100 Subject: [PATCH 4/6] Make implementation follow assignment rules From what I could find, the rules of the assignment seem to be: 1. Restrict oneself to at most *one* O(log(N)) call, and otherwise use constant time operations on the map. 2. Don't use more operations than strictly necessary on `K` and `V`. 3. Prefer simplicity to performance. I think my solution would fare well under those constraints. --- 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_{}; }; From 3305b139367daefd3e60c185aeaa28acb56d686d Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Sat, 24 Aug 2024 20:37:27 +0100 Subject: [PATCH 5/6] Fix Meson install --- src/meson.build | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/meson.build b/src/meson.build index 0e8c25d..3c30a37 100644 --- a/src/meson.build +++ b/src/meson.build @@ -1 +1,3 @@ interval_map_dep = declare_dependency(include_directories: 'include') + +install_headers('include/interval-map/interval-map.hh', subdir: 'interval-map') From 2004cd06cfc2a60e4229b242dd4b7ed2cc4b83d6 Mon Sep 17 00:00:00 2001 From: Bruno BELANYI Date: Sat, 24 Aug 2024 20:59:14 +0100 Subject: [PATCH 6/6] Fix edge-case in identify 'end' upper-bound Just my luck, this was found immediately in the CI, right when I uploaded the project thinking I was done. Thankfully the fix is easy and (in hindsight) quite obvious. --- src/include/interval-map/interval-map.hh | 2 +- tests/unit/unit_test.cc | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/include/interval-map/interval-map.hh b/src/include/interval-map/interval-map.hh index ab3fb44..b518f24 100644 --- a/src/include/interval-map/interval-map.hh +++ b/src/include/interval-map/interval-map.hh @@ -35,7 +35,7 @@ public: if (insert_begin) it = underlying_.insert(it, {begin, val}); // Get the proper upper-bound for `end` - it = (it == underlying_.end()) ? it : std::next(it); + it = (it == underlying_.end() || end < it->first) ? it : std::next(it); if (!(at_upper_bound(it) == end_val)) underlying_.insert(it, {end, end_val}); } diff --git a/tests/unit/unit_test.cc b/tests/unit/unit_test.cc index c429d29..53675c3 100644 --- a/tests/unit/unit_test.cc +++ b/tests/unit/unit_test.cc @@ -242,6 +242,12 @@ TEST_F(IntervalMapTest, fuzzing_003) { assign(-110, -10, 4); } +TEST_F(IntervalMapTest, fuzzing_004) { + assign(-20, 120, 1); + assign(50, 110, 0); + assign(-120, 100, 0); +} + TEST_F(IntervalMapTest, randomized_test) { auto const seed = []() { std::random_device r;