From c242aeeb6926f224ff84dd7e72ddbf1562cbb821 Mon Sep 17 00:00:00 2001 From: "J. Eric Ivancich" Date: Mon, 9 Jan 2017 15:30:37 -0500 Subject: [PATCH] Use value equality (operator==) when finding by value (used to be by address). Add tests. Signed-off-by: J. Eric Ivancich --- support/src/indirect_intrusive_heap.h | 14 ++--- support/test/test_indirect_intrusive_heap.cc | 54 ++++++++++++++++---- 2 files changed, 50 insertions(+), 18 deletions(-) diff --git a/support/src/indirect_intrusive_heap.h b/support/src/indirect_intrusive_heap.h index 96d2228..c3b4dd5 100644 --- a/support/src/indirect_intrusive_heap.h +++ b/support/src/indirect_intrusive_heap.h @@ -251,19 +251,19 @@ namespace crimson { i = end(); } - Iterator find(const I& item) { + Iterator find(const I& ind_item) { for (HeapIndex i = 0; i < count; ++i) { - if (data[i] == item) { + if (data[i] == ind_item) { return Iterator(*this, i); } } return end(); } - // NB: should we be using operator== instead of address check? + // when passing in value we do a comparison via operator== Iterator find(const T& item) { for (HeapIndex i = 0; i < count; ++i) { - if (data[i].get() == &item) { + if (*data[i] == item) { return Iterator(*this, i); } } @@ -271,11 +271,11 @@ namespace crimson { } // reverse find -- start looking from bottom of heap - Iterator rfind(const I& item) { + Iterator rfind(const I& ind_item) { // HeapIndex is unsigned, so we can't allow to go negative; so // we'll keep it one more than actual index for (HeapIndex i = count; i > 0; --i) { - if (data[i-1] == item) { + if (data[i-1] == ind_item) { return Iterator(*this, i-1); } } @@ -287,7 +287,7 @@ namespace crimson { // HeapIndex is unsigned, so we can't allow to go negative; so // we'll keep it one more than actual index for (HeapIndex i = count; i > 0; --i) { - if (data[i-1].get() == &item) { + if (*data[i-1] == item) { return Iterator(*this, i-1); } } diff --git a/support/test/test_indirect_intrusive_heap.cc b/support/test/test_indirect_intrusive_heap.cc index df4c2a8..143998a 100644 --- a/support/test/test_indirect_intrusive_heap.cc +++ b/support/test/test_indirect_intrusive_heap.cc @@ -22,6 +22,10 @@ struct Elem { Elem(int _data) : data(_data) { } + bool operator==(const Elem& other) { + return data == other.data; + } + friend std::ostream& operator<<(std::ostream& out, const Elem& d) { out << d.data; return out; @@ -55,7 +59,7 @@ struct ElemCompareAlt { }; -class HeapFixture1: public ::testing::Test { +class HeapFixture1: public ::testing::Test { public: @@ -84,7 +88,7 @@ class HeapFixture1: public ::testing::Test { heap.push(data7); } - void TearDown() { + void TearDown() { // nothing to do } }; // class HeapFixture1 @@ -412,7 +416,7 @@ TEST(IndIntruHeap, multi_K) { bound = current; } - + EXPECT_TRUE(heap2.empty()) << "should be empty after all elements popped"; EXPECT_TRUE(heap3.empty()) << "should be empty after all elements popped"; EXPECT_TRUE(heap4.empty()) << "should be empty after all elements popped"; @@ -660,7 +664,7 @@ TEST_F(HeapFixture1, iterator_basics) { } auto i1 = heap.begin(); - + EXPECT_EQ(-12, i1->data) << "first member with * operator must be smallest"; @@ -705,7 +709,7 @@ TEST_F(HeapFixture1, const_iterator_basics) { } auto i1 = heap.cbegin(); - + EXPECT_EQ(-12, i1->data) << "first member with * operator must be smallest"; @@ -740,26 +744,54 @@ TEST_F(HeapFixture1, const_iterator_basics) { TEST_F(HeapFixture1, iterator_find_rfind) { { auto it1 = heap.find(data7); - EXPECT_NE(heap.end(), it1) << "find for included element should succeed"; + EXPECT_NE(heap.end(), it1) << + "find by indirection for included element should succeed"; EXPECT_EQ(-7, it1->data) << - "find for included element should result in right value"; + "find by indirection for included element should result in right value"; auto fake_data = std::make_shared(-7); auto it2 = heap.find(fake_data); - EXPECT_EQ(heap.end(), it2) << "find for not included element should fail"; + EXPECT_EQ(heap.end(), it2) << + "find by indirection for not included element should fail"; + } + + { + auto it1 = heap.find(Elem(-7)); + EXPECT_NE(heap.end(), it1) << + "find by value for included element should succeed"; + EXPECT_EQ(-7, it1->data) << + "find by value for included element should result in right value"; + + auto it2 = heap.find(Elem(7)); + EXPECT_EQ(heap.end(), it2) << + "find by value for not included element should fail"; } { auto it1 = heap.rfind(data7); EXPECT_NE(heap.end(), it1) << - "reverse find for included element should succeed"; + "reverse find by indirecton for included element should succeed"; EXPECT_EQ(-7, it1->data) << - "reverse find for included element should result in right value"; + "reverse find by indirection for included element should result " + "in right value"; auto fake_data = std::make_shared(-7); auto it2 = heap.rfind(fake_data); EXPECT_EQ(heap.end(), it2) << - "reverse find for not included element should fail"; + "reverse find by indirection for not included element should fail"; + } + + { + auto it1 = heap.rfind(Elem(-7)); + EXPECT_NE(heap.end(), it1) << + "reverse find by value for included element should succeed"; + EXPECT_EQ(-7, it1->data) << + "reverse find by value for included element should result " + "in right value"; + + auto it2 = heap.rfind(Elem(7)); + EXPECT_EQ(heap.end(), it2) << + "reverse find by value for not included element should fail"; } }