Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unordered and ordered containers ranges for empty containers #673

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions include/oneapi/tbb/detail/_concurrent_skip_list.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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; }
Expand Down
4 changes: 2 additions & 2 deletions include/oneapi/tbb/detail/_concurrent_unordered_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,7 @@ class concurrent_unordered_base {
using difference_type = typename concurrent_unordered_base::difference_type;
using iterator = typename concurrent_unordered_base::const_iterator;

bool empty() const { return my_begin_node == my_end_node; }
bool empty() const { return my_instance.first_value_node(my_begin_node) == my_end_node; }

bool is_divisible() const {
return my_midpoint_node != my_end_node;
Expand Down Expand Up @@ -733,7 +733,7 @@ class concurrent_unordered_base {
}
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);
Expand Down
39 changes: 39 additions & 0 deletions test/common/concurrent_associative_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,26 @@ 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");

Range r2(r, tbb::split{});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not stated if split ctor can be used for non-divisible range; however, the spec says about is_divisible: "True if the range can be partitioned into two subranges.". I.e. range cannot be partitioned if false. Does it mean that we cannot use splitting ctor to partition the empty range?

REQUIRE_MESSAGE(r.empty(), "Empty container range should be empty");
REQUIRE_MESSAGE(r2.empty(), "Empty container range should be empty");
REQUIRE_MESSAGE(!r.is_divisible(), "Empty container range should not be divisible");
REQUIRE_MESSAGE(!r2.is_divisible(), "Empty container range should not be divisible");
REQUIRE_MESSAGE(r.begin() == r.end(), "Incorrect iterators on empty range");
REQUIRE_MESSAGE(r2.begin() == r2.end(), "Incorrect iterators on empty range");
REQUIRE_MESSAGE(r.begin() == cont.begin(), "Incorrect iterators on empty range");
REQUIRE_MESSAGE(r2.begin() == cont.begin(), "Incorrect iterators on empty range");
}

template<typename T, typename CheckElementState>
void test_basic_common()
{
Expand All @@ -446,6 +466,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");
Expand Down