From 5a136080d676affdba3d17190cdf41b6e5e6be79 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 1 Feb 2018 13:22:37 -0500 Subject: [PATCH 1/4] Code coverage cleanup -- remove lines not hit, or move them to tests. Signed-off-by: Joshua Marantz --- source/common/common/shared_memory_hash_set.h | 53 +++++-------------- .../common/shared_memory_hash_set_test.cc | 24 ++++++++- 2 files changed, 37 insertions(+), 40 deletions(-) diff --git a/source/common/common/shared_memory_hash_set.h b/source/common/common/shared_memory_hash_set.h index 713076c923b13..b19ef43f36ce8 100644 --- a/source/common/common/shared_memory_hash_set.h +++ b/source/common/common/shared_memory_hash_set.h @@ -18,9 +18,6 @@ namespace Envoy { * an existing memory segment. */ struct SharedMemoryHashSetOptions { - std::string toString() const { - return fmt::format("capacity={}, num_slots={}", capacity, num_slots); - } std::string version() const { return fmt::format("capacity={}, num_slots={}", capacity, num_slots); } @@ -101,58 +98,39 @@ template class SharedMemoryHashSet : public Logger::Loggablesize <= control_->options.capacity); // As a sanity check, make sure there are control_->size values - // reachable from the slots, each of which has a valid char_offset + // reachable from the slots, each of which has a valid + // char_offset. + // + // Avoid infinite loops if there is a next_cell cycle within a + // slot. Note that the num_values message will be emitted outside + // the loop. uint32_t num_values = 0; for (uint32_t slot = 0; slot < control_->options.num_slots; ++slot) { uint32_t next = 0; // initialized to silence compilers. - for (uint32_t cell_index = slots_[slot]; cell_index != Sentinal; cell_index = next) { + for (uint32_t cell_index = slots_[slot]; + (cell_index != Sentinal) && (num_values <= control_->size); cell_index = next) { RELEASE_ASSERT(cell_index < control_->options.capacity); Cell& cell = getCell(cell_index); absl::string_view key = cell.value.key(); RELEASE_ASSERT(computeSlot(key) == slot); next = cell.next_cell; ++num_values; - // Avoid infinite loops if there is a next_cell cycle within - // a slot. Note that the num_values message will be emitted - // outside the loop. - if (num_values > control_->size) { - break; - } } } RELEASE_ASSERT(num_values == control_->size); uint32_t num_free_entries = 0; uint32_t expected_free_entries = control_->options.capacity - control_->size; - for (uint32_t cell_index = control_->free_cell_index; cell_index != Sentinal; + + // Don't infinite-loop with a corruption; break when we see there's a problem. + for (uint32_t cell_index = control_->free_cell_index; + (cell_index != Sentinal) && (num_free_entries <= expected_free_entries); cell_index = getCell(cell_index).next_cell) { ++num_free_entries; - if (num_free_entries > expected_free_entries) { - // Don't infinite-loop with a corruption; break when we see there's a problem. - break; - } } RELEASE_ASSERT(num_free_entries == expected_free_entries); } - /** - * Returns a string describing the contents of the map, including the control - * bits and the keys in each slot. - */ - std::string toString() { - std::string ret; - ret = - fmt::format("options={}\ncontrol={}\n", control_->options.toString(), control_->toString()); - for (uint32_t i = 0; i < control_->options.num_slots; ++i) { - ret += fmt::format("slot {}:", i); - for (uint32_t j = slots_[i]; j != Sentinal; j = getCell(j).next_cell) { - ret += " " + std::string(getCell(j).value.key()); - } - ret += "\n"; - } - return ret; - } - /** * Inserts a value into the set. If successful (e.g. map has * capacity) then put returns a pointer to the value object, which @@ -241,6 +219,8 @@ template class SharedMemoryHashSet : public Logger::Loggable class SharedMemoryHashSet : public Logger::Loggable + std::string hashSetToString(SharedMemoryHashSet& hs) { + std::string ret; + static const uint32_t sentinal = SharedMemoryHashSet::Sentinal; + std::string control_string = + fmt::format("{} size={} free_cell_index={}", hs.control_->options.version(), + hs.control_->size, hs.control_->free_cell_index); + ret = fmt::format("options={}\ncontrol={}\n", hs.control_->options.version(), control_string); + for (uint32_t i = 0; i < hs.control_->options.num_slots; ++i) { + ret += fmt::format("slot {}:", i); + for (uint32_t j = hs.slots_[i]; j != sentinal; j = hs.getCell(j).next_cell) { + ret += " " + std::string(hs.getCell(j).value.key()); + } + ret += "\n"; + } + return ret; + } + SharedMemoryHashSetOptions options_; std::unique_ptr memory_; }; @@ -102,7 +124,7 @@ TEST_F(SharedMemoryHashSetTest, putRemove) { SharedMemoryHashSet hash_set2(options_, false, memory_.get()); EXPECT_EQ(1, hash_set2.size()); EXPECT_EQ(nullptr, hash_set2.get("no such key")); - EXPECT_EQ(6789, hash_set2.get("good key")->number) << hash_set2.toString(); + EXPECT_EQ(6789, hash_set2.get("good key")->number) << hashSetToString(hash_set2); EXPECT_FALSE(hash_set2.remove("no such key")); hash_set2.sanityCheck(); EXPECT_TRUE(hash_set2.remove("good key")); From 7d3345e908d03679f5aba9b7ce3d9e016be0b5a7 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 1 Feb 2018 13:29:33 -0500 Subject: [PATCH 2/4] Settle on toString() and the serializer for Options, rather than version(). Signed-off-by: Joshua Marantz --- source/common/common/shared_memory_hash_set.h | 2 +- test/common/common/shared_memory_hash_set_test.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/common/shared_memory_hash_set.h b/source/common/common/shared_memory_hash_set.h index b19ef43f36ce8..5af93469f97ea 100644 --- a/source/common/common/shared_memory_hash_set.h +++ b/source/common/common/shared_memory_hash_set.h @@ -18,7 +18,7 @@ namespace Envoy { * an existing memory segment. */ struct SharedMemoryHashSetOptions { - std::string version() const { + std::string toString() const { return fmt::format("capacity={}, num_slots={}", capacity, num_slots); } bool operator==(const SharedMemoryHashSetOptions& that) const { diff --git a/test/common/common/shared_memory_hash_set_test.cc b/test/common/common/shared_memory_hash_set_test.cc index f56c230e2c9fc..a50ea89c4c15a 100644 --- a/test/common/common/shared_memory_hash_set_test.cc +++ b/test/common/common/shared_memory_hash_set_test.cc @@ -59,9 +59,9 @@ class SharedMemoryHashSetTest : public testing::Test { std::string ret; static const uint32_t sentinal = SharedMemoryHashSet::Sentinal; std::string control_string = - fmt::format("{} size={} free_cell_index={}", hs.control_->options.version(), + fmt::format("{} size={} free_cell_index={}", hs.control_->options.toString(), hs.control_->size, hs.control_->free_cell_index); - ret = fmt::format("options={}\ncontrol={}\n", hs.control_->options.version(), control_string); + ret = fmt::format("options={}\ncontrol={}\n", hs.control_->options.toString(), control_string); for (uint32_t i = 0; i < hs.control_->options.num_slots; ++i) { ret += fmt::format("slot {}:", i); for (uint32_t j = hs.slots_[i]; j != sentinal; j = hs.getCell(j).next_cell) { From 70588da3c115871a49027806a0f362f17b7aab56 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 1 Feb 2018 13:22:37 -0500 Subject: [PATCH 3/4] Code coverage cleanup -- remove lines not hit, or move them to tests. Signed-off-by: Joshua Marantz --- source/common/common/shared_memory_hash_set.h | 53 +++++-------------- .../common/shared_memory_hash_set_test.cc | 24 ++++++++- 2 files changed, 37 insertions(+), 40 deletions(-) diff --git a/source/common/common/shared_memory_hash_set.h b/source/common/common/shared_memory_hash_set.h index 713076c923b13..b19ef43f36ce8 100644 --- a/source/common/common/shared_memory_hash_set.h +++ b/source/common/common/shared_memory_hash_set.h @@ -18,9 +18,6 @@ namespace Envoy { * an existing memory segment. */ struct SharedMemoryHashSetOptions { - std::string toString() const { - return fmt::format("capacity={}, num_slots={}", capacity, num_slots); - } std::string version() const { return fmt::format("capacity={}, num_slots={}", capacity, num_slots); } @@ -101,58 +98,39 @@ template class SharedMemoryHashSet : public Logger::Loggablesize <= control_->options.capacity); // As a sanity check, make sure there are control_->size values - // reachable from the slots, each of which has a valid char_offset + // reachable from the slots, each of which has a valid + // char_offset. + // + // Avoid infinite loops if there is a next_cell cycle within a + // slot. Note that the num_values message will be emitted outside + // the loop. uint32_t num_values = 0; for (uint32_t slot = 0; slot < control_->options.num_slots; ++slot) { uint32_t next = 0; // initialized to silence compilers. - for (uint32_t cell_index = slots_[slot]; cell_index != Sentinal; cell_index = next) { + for (uint32_t cell_index = slots_[slot]; + (cell_index != Sentinal) && (num_values <= control_->size); cell_index = next) { RELEASE_ASSERT(cell_index < control_->options.capacity); Cell& cell = getCell(cell_index); absl::string_view key = cell.value.key(); RELEASE_ASSERT(computeSlot(key) == slot); next = cell.next_cell; ++num_values; - // Avoid infinite loops if there is a next_cell cycle within - // a slot. Note that the num_values message will be emitted - // outside the loop. - if (num_values > control_->size) { - break; - } } } RELEASE_ASSERT(num_values == control_->size); uint32_t num_free_entries = 0; uint32_t expected_free_entries = control_->options.capacity - control_->size; - for (uint32_t cell_index = control_->free_cell_index; cell_index != Sentinal; + + // Don't infinite-loop with a corruption; break when we see there's a problem. + for (uint32_t cell_index = control_->free_cell_index; + (cell_index != Sentinal) && (num_free_entries <= expected_free_entries); cell_index = getCell(cell_index).next_cell) { ++num_free_entries; - if (num_free_entries > expected_free_entries) { - // Don't infinite-loop with a corruption; break when we see there's a problem. - break; - } } RELEASE_ASSERT(num_free_entries == expected_free_entries); } - /** - * Returns a string describing the contents of the map, including the control - * bits and the keys in each slot. - */ - std::string toString() { - std::string ret; - ret = - fmt::format("options={}\ncontrol={}\n", control_->options.toString(), control_->toString()); - for (uint32_t i = 0; i < control_->options.num_slots; ++i) { - ret += fmt::format("slot {}:", i); - for (uint32_t j = slots_[i]; j != Sentinal; j = getCell(j).next_cell) { - ret += " " + std::string(getCell(j).value.key()); - } - ret += "\n"; - } - return ret; - } - /** * Inserts a value into the set. If successful (e.g. map has * capacity) then put returns a pointer to the value object, which @@ -241,6 +219,8 @@ template class SharedMemoryHashSet : public Logger::Loggable class SharedMemoryHashSet : public Logger::Loggable + std::string hashSetToString(SharedMemoryHashSet& hs) { + std::string ret; + static const uint32_t sentinal = SharedMemoryHashSet::Sentinal; + std::string control_string = + fmt::format("{} size={} free_cell_index={}", hs.control_->options.version(), + hs.control_->size, hs.control_->free_cell_index); + ret = fmt::format("options={}\ncontrol={}\n", hs.control_->options.version(), control_string); + for (uint32_t i = 0; i < hs.control_->options.num_slots; ++i) { + ret += fmt::format("slot {}:", i); + for (uint32_t j = hs.slots_[i]; j != sentinal; j = hs.getCell(j).next_cell) { + ret += " " + std::string(hs.getCell(j).value.key()); + } + ret += "\n"; + } + return ret; + } + SharedMemoryHashSetOptions options_; std::unique_ptr memory_; }; @@ -102,7 +124,7 @@ TEST_F(SharedMemoryHashSetTest, putRemove) { SharedMemoryHashSet hash_set2(options_, false, memory_.get()); EXPECT_EQ(1, hash_set2.size()); EXPECT_EQ(nullptr, hash_set2.get("no such key")); - EXPECT_EQ(6789, hash_set2.get("good key")->number) << hash_set2.toString(); + EXPECT_EQ(6789, hash_set2.get("good key")->number) << hashSetToString(hash_set2); EXPECT_FALSE(hash_set2.remove("no such key")); hash_set2.sanityCheck(); EXPECT_TRUE(hash_set2.remove("good key")); From 88ebd441c5ff6e85ab7f41dc8213729e4519ba46 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 1 Feb 2018 13:29:33 -0500 Subject: [PATCH 4/4] Settle on toString() and the serializer for Options, rather than version(). Signed-off-by: Joshua Marantz --- source/common/common/shared_memory_hash_set.h | 2 +- test/common/common/shared_memory_hash_set_test.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/common/shared_memory_hash_set.h b/source/common/common/shared_memory_hash_set.h index b19ef43f36ce8..5af93469f97ea 100644 --- a/source/common/common/shared_memory_hash_set.h +++ b/source/common/common/shared_memory_hash_set.h @@ -18,7 +18,7 @@ namespace Envoy { * an existing memory segment. */ struct SharedMemoryHashSetOptions { - std::string version() const { + std::string toString() const { return fmt::format("capacity={}, num_slots={}", capacity, num_slots); } bool operator==(const SharedMemoryHashSetOptions& that) const { diff --git a/test/common/common/shared_memory_hash_set_test.cc b/test/common/common/shared_memory_hash_set_test.cc index f56c230e2c9fc..a50ea89c4c15a 100644 --- a/test/common/common/shared_memory_hash_set_test.cc +++ b/test/common/common/shared_memory_hash_set_test.cc @@ -59,9 +59,9 @@ class SharedMemoryHashSetTest : public testing::Test { std::string ret; static const uint32_t sentinal = SharedMemoryHashSet::Sentinal; std::string control_string = - fmt::format("{} size={} free_cell_index={}", hs.control_->options.version(), + fmt::format("{} size={} free_cell_index={}", hs.control_->options.toString(), hs.control_->size, hs.control_->free_cell_index); - ret = fmt::format("options={}\ncontrol={}\n", hs.control_->options.version(), control_string); + ret = fmt::format("options={}\ncontrol={}\n", hs.control_->options.toString(), control_string); for (uint32_t i = 0; i < hs.control_->options.num_slots; ++i) { ret += fmt::format("slot {}:", i); for (uint32_t j = hs.slots_[i]; j != sentinal; j = hs.getCell(j).next_cell) {