Fix unordered and ordered containers ranges for empty containers (#703)

* Fix container::range_type behavior in case of empty containers * Remove test that violates the spec * Fix range.begin to point to a value node Signed-off-by: Alexei Katranov <[email protected]> * Remove first_value_node from empty Co-authored-by: Alexei Katranov <[email protected]>
oneapi-src · Jan 21, 2022 · 555bc87 · 555bc87
1 parent 1bfd90d
commit 555bc87
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 8 deletions.
diff --git a/include/oneapi/tbb/detail/_concurrent_skip_list.h b/include/oneapi/tbb/detail/_concurrent_skip_list.h
@@ -72,7 +72,7 @@ class skip_list_node {
     using pointer = typename value_allocator_traits::pointer;
     using const_pointer = typename value_allocator_traits::const_pointer;
 
-    //In perfect world these constructor and destructor would have been private, 
+    //In perfect world these constructor and destructor would have been private,
     //however this seems technically impractical due to use of allocator_traits.
 
     //Should not be called directly, instead use create method
@@ -671,24 +671,34 @@ class concurrent_skip_list {
         using reference = typename iterator::reference;
 
         bool empty() const {
-            return my_begin.my_node_ptr->next(0) == my_end.my_node_ptr;
+            return my_begin.my_node_ptr ? (my_begin.my_node_ptr->next(0) == my_end.my_node_ptr)
+                                        : true;
         }
 
         bool is_divisible() const {
-            return my_level != 0 ? my_begin.my_node_ptr->next(my_level - 1) != my_end.my_node_ptr : false;
+            return my_begin.my_node_ptr && my_level != 0
+                        ? my_begin.my_node_ptr->next(my_level - 1) != my_end.my_node_ptr
+                        : false;
         }
 
         size_type size() const { return std::distance(my_begin, my_end); }
 
         const_range_type( const_range_type& r, split)
             : my_end(r.my_end) {
-            my_begin = iterator(r.my_begin.my_node_ptr->next(r.my_level - 1));
-            my_level = my_begin.my_node_ptr->height();
+            if (r.empty()) {
+                __TBB_ASSERT(my_end.my_node_ptr == nullptr, nullptr);
+                my_begin = my_end;
+                my_level = 0;
+            } else {
+                my_begin = iterator(r.my_begin.my_node_ptr->next(r.my_level - 1));
+                my_level = my_begin.my_node_ptr->height();
+            }
             r.my_end = my_begin;
         }
 
         const_range_type( const concurrent_skip_list& l)
-            : my_end(l.end()), my_begin(l.begin()), my_level(my_begin.my_node_ptr->height() ) {}
+            : my_end(l.end()), my_begin(l.begin()),
+              my_level(my_begin.my_node_ptr ? my_begin.my_node_ptr->height() : 0) {}
 
         iterator begin() const { return my_begin; }
         iterator end() const { return my_end; }

diff --git a/include/oneapi/tbb/detail/_concurrent_unordered_base.h b/include/oneapi/tbb/detail/_concurrent_unordered_base.h
@@ -727,13 +727,13 @@ class concurrent_unordered_base {
         iterator end() const { return iterator(my_instance.first_value_node(my_end_node)); }
 
         const_range_type( const concurrent_unordered_base& table )
-            : my_instance(table), my_begin_node(const_cast<node_ptr>(&table.my_head)), my_end_node(nullptr)
+            : my_instance(table), my_begin_node(my_instance.first_value_node(const_cast<node_ptr>(&table.my_head))), my_end_node(nullptr)
         {
             set_midpoint();
         }
     private:
         void set_midpoint() const {
-            if (my_begin_node == my_end_node) {
+            if (empty()) {
                 my_midpoint_node = my_end_node;
             } else {
                 sokey_type invalid_key = ~sokey_type(0);

diff --git a/test/common/concurrent_associative_common.h b/test/common/concurrent_associative_common.h
@@ -425,6 +425,16 @@ void test_comparison_operators() {
     check_unequal(cont, cont6);
 }
 
+template <typename Range, typename Container>
+void test_empty_container_range(Container&& cont) {
+    REQUIRE(cont.empty());
+    Range r = cont.range();
+    REQUIRE_MESSAGE(r.empty(), "Empty container range should be empty");
+    REQUIRE_MESSAGE(!r.is_divisible(), "Empty container range should not be divisible");
+    REQUIRE_MESSAGE(r.begin() == r.end(), "Incorrect iterators on empty range");
+    REQUIRE_MESSAGE(r.begin() == cont.begin(), "Incorrect iterators on empty range");
+}
+
 template<typename T, typename CheckElementState>
 void test_basic_common()
 {
@@ -446,6 +456,25 @@ void test_basic_common()
     REQUIRE_MESSAGE(ccont.begin() == ccont.end(), "Concurrent container iterators are invalid after construction");
     REQUIRE_MESSAGE(cont.cbegin() == cont.cend(), "Concurrent container iterators are invalid after construction");
 
+    // Test range for empty container
+    using range_type = typename T::range_type;
+    using const_range_type = typename T::const_range_type;
+    test_empty_container_range<range_type>(cont);
+    test_empty_container_range<const_range_type>(cont);
+    test_empty_container_range<const_range_type>(ccont);
+
+    T empty_cont;
+    const T& empty_ccont = empty_cont;
+
+    for (int i = 0; i < 1000; ++i) {
+        empty_cont.insert(Value<T>::make(i));
+    }
+    empty_cont.clear();
+
+    test_empty_container_range<range_type>(empty_cont);
+    test_empty_container_range<const_range_type>(empty_cont);
+    test_empty_container_range<const_range_type>(empty_ccont);
+
     //std::pair<iterator, bool> insert(const value_type& obj);
     std::pair<typename T::iterator, bool> ins = cont.insert(Value<T>::make(1));
     REQUIRE_MESSAGE((ins.second == true && Value<T>::get(*(ins.first)) == 1), "Element 1 has not been inserted properly");