From 615f8cd443b2b31424b3a07cf499bfcc1bd0f934 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 30 Apr 2020 13:04:38 -0700 Subject: [PATCH 1/5] Fix bug with List where replace() doesn't work with non-pointer types --- src/List.h | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/List.h b/src/List.h index d4b9798e4d..57d26c462b 100644 --- a/src/List.h +++ b/src/List.h @@ -240,16 +240,16 @@ public: T replace(int ent_index, const T& new_ent) // replace entry #i with a new value { if ( ent_index < 0 ) - return 0; + return get_default_value(); - T old_ent = nullptr; + T old_ent = get_default_value(); if ( ent_index > num_entries - 1 ) { // replacement beyond the end of the list resize(ent_index + 1); for ( int i = num_entries; i < max_entries; ++i ) - entries[i] = nullptr; + entries[i] = get_default_value(); num_entries = max_entries; } else @@ -287,6 +287,20 @@ public: protected: + T get_default_value() + { + if constexpr ( std::is_pointer_v ) + return nullptr; + else if constexpr ( std::is_integral_v ) + return 0; + else if constexpr ( std::is_floating_point_v ) + return 0.0; + else + // TODO: Should this return an error at compile time that we don't + // know what this type is? + return T(); + } + // This could essentially be an std::vector if we wanted. Some // reasons to maybe not refactor to use std::vector ? // From 0558a7bfed976bfeef3a5d7cea6b40e9b650197b Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 30 Apr 2020 13:12:03 -0700 Subject: [PATCH 2/5] Add unit testing for List --- src/CMakeLists.txt | 1 + src/List.cc | 116 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+) create mode 100644 src/List.cc diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 8e533f5733..facc913a59 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -246,6 +246,7 @@ set(MAIN_SRCS IntSet.cc IP.cc IPAddr.cc + List.cc Reporter.cc NFA.cc Net.cc diff --git a/src/List.cc b/src/List.cc new file mode 100644 index 0000000000..8d70cc95c2 --- /dev/null +++ b/src/List.cc @@ -0,0 +1,116 @@ +#include +#include <3rdparty/doctest.h> + +TEST_CASE("list construction") + { + List list; + CHECK(list.empty()); + + List list2(10); + CHECK(list2.empty()); + CHECK(list2.max() == 10); + } + +TEST_CASE("list operation") + { + List list({ 1, 2, 3 }); + CHECK(list.size() == 3); + CHECK(list.max() == 3); + CHECK(list[0] == 1); + CHECK(list[1] == 2); + CHECK(list[2] == 3); + + // push_back forces a resize of the list here, which grows the list + // by a growth factor. That makes the max elements equal to 6. + list.push_back(4); + CHECK(list.size() == 4); + CHECK(list.max() == 6); + CHECK(list[3] == 4); + + CHECK(list.front() == 1); + CHECK(list.back() == 4); + + list.pop_front(); + CHECK(list.size() == 3); + CHECK(list.front() == 2); + + list.pop_back(); + CHECK(list.size() == 2); + CHECK(list.back() == 3); + + list.push_back(4); + CHECK(list.is_member(2)); + CHECK(list.member_pos(2) == 0); + + list.remove(2); + CHECK(list.size() == 2); + CHECK(list[0] == 3); + CHECK(list[1] == 4); + + // Squash the list down to the existing elements. + list.resize(); + CHECK(list.size() == 2); + CHECK(list.max() == 2); + + // Attempt replacing a known position. + int old = list.replace(0, 10); + CHECK(list.size() == 2); + CHECK(list.max() == 2); + CHECK(old == 3); + CHECK(list[0] == 10); + CHECK(list[1] == 4); + + // Attempt replacing an element off the end of the list, which + // causes a resize. + old = list.replace(3, 5); + CHECK(list.size() == 4); + CHECK(list.max() == 4); + CHECK(old == 0); + CHECK(list[0] == 10); + CHECK(list[1] == 4); + CHECK(list[2] == 0); + CHECK(list[3] == 5); + + // Attempt replacing an element with a negative index, which returns the + // default value for the list type. + old = list.replace(-1, 50); + CHECK(list.size() == 4); + CHECK(list.max() == 4); + CHECK(old == 0); + + list.clear(); + CHECK(list.size() == 0); + CHECK(list.max() == 0); + } + +TEST_CASE("list iteration") + { + List list({ 1, 2, 3, 4}); + + int index = 1; + for ( int v : list ) + CHECK(v == index++); + + index = 1; + for ( auto it = list.begin(); it != list.end(); index++, ++it) + CHECK(*it == index); + } + +TEST_CASE("plists") + { + PList list; + list.push_back(new int(1)); + list.push_back(new int(2)); + list.push_back(new int(3)); + + CHECK(*list[0] == 1); + + int* new_val = new int(5); + auto old = list.replace(-1, new_val); + delete new_val; + CHECK(old == nullptr); + + for ( auto v : list ) + delete v; + list.clear(); + } From 28e510084264f09304610ff6d1ac8a839ffe243e Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 30 Apr 2020 13:26:00 -0700 Subject: [PATCH 3/5] Add ability for List to be ordered/unordered This fixes a "bug" with List where remove_nth() can be an O(n) operation when it doesn't need to be. remove_nth for lists that don't necessarily need to keep an order can be an O(1) operation instead. --- src/List.cc | 14 ++++++++++++++ src/List.h | 36 ++++++++++++++++++++++++------------ 2 files changed, 38 insertions(+), 12 deletions(-) diff --git a/src/List.cc b/src/List.cc index 8d70cc95c2..1f2e4ec410 100644 --- a/src/List.cc +++ b/src/List.cc @@ -114,3 +114,17 @@ TEST_CASE("plists") delete v; list.clear(); } + +TEST_CASE("unordered list operation") + { + List list({1, 2, 3, 4}); + CHECK(list.size() == 4); + + // An unordered list doesn't maintain the ordering of the elements when + // one is removed. It just swaps the last element into the hole. + list.remove(2); + CHECK(list.size() == 3); + CHECK(list[0] == 1); + CHECK(list[1] == 4); + CHECK(list[2] == 3); + } diff --git a/src/List.h b/src/List.h index 57d26c462b..55256b35fb 100644 --- a/src/List.h +++ b/src/List.h @@ -29,7 +29,9 @@ // TODO: this can be removed in v3.1 when List::sort() is removed typedef int (*list_cmp_func)(const void* v1, const void* v2); -template +enum list_order { LIST_ORDERED, LIST_UNORDERED }; + +template class List { public: @@ -195,13 +197,11 @@ public: bool remove(const T& a) // delete entry from list { - for ( int i = 0; i < num_entries; ++i ) + int pos = member_pos(a); + if ( pos != -1 ) { - if ( a == entries[i] ) - { - remove_nth(i); - return true; - } + remove_nth(pos); + return true; } return false; @@ -212,10 +212,22 @@ public: assert(n >=0 && n < num_entries); T old_ent = entries[n]; - --num_entries; - for ( ; n < num_entries; ++n ) - entries[n] = entries[n+1]; + // For data where we don't care about ordering, we don't care about keeping + // the list in the same order when removing an element. Just swap the last + // element with the element being removed. + if ( Order == LIST_ORDERED ) + { + --num_entries; + + for ( ; n < num_entries; ++n ) + entries[n] = entries[n+1]; + } + else + { + entries[n] = entries[num_entries - 1]; + --num_entries; + } return old_ent; } @@ -332,8 +344,8 @@ protected: // Specialization of the List class to store pointers of a type. -template -using PList = List; +template +using PList = List; // Popular type of list: list of strings. typedef PList name_list; From 499a3353b53a1e3828ee3da9075c8176b37c4b7f Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 30 Apr 2020 13:27:29 -0700 Subject: [PATCH 4/5] Change timer_list in BroList to be an unordered list. This type is used by Conn and Analyzer to hold onto timers being added and removed. We don't expect the elements in those lists to maintain an order as the list is being modified. --- src/BroList.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/BroList.h b/src/BroList.h index 48e9ebee04..c7637b6bd0 100644 --- a/src/BroList.h +++ b/src/BroList.h @@ -23,4 +23,4 @@ class Attr; typedef PList attr_list; class Timer; -typedef PList timer_list; +typedef PList timer_list; From 690061b01c6d9de90a1552370819d4a309ee3293 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 7 May 2020 17:25:26 -0700 Subject: [PATCH 5/5] Review fixups --- src/BroList.h | 2 +- src/List.cc | 2 +- src/List.h | 28 +++++++--------------------- 3 files changed, 9 insertions(+), 23 deletions(-) diff --git a/src/BroList.h b/src/BroList.h index c7637b6bd0..a37f40d84a 100644 --- a/src/BroList.h +++ b/src/BroList.h @@ -23,4 +23,4 @@ class Attr; typedef PList attr_list; class Timer; -typedef PList timer_list; +typedef PList timer_list; diff --git a/src/List.cc b/src/List.cc index 1f2e4ec410..c19353b341 100644 --- a/src/List.cc +++ b/src/List.cc @@ -117,7 +117,7 @@ TEST_CASE("plists") TEST_CASE("unordered list operation") { - List list({1, 2, 3, 4}); + List list({1, 2, 3, 4}); CHECK(list.size() == 4); // An unordered list doesn't maintain the ordering of the elements when diff --git a/src/List.h b/src/List.h index 55256b35fb..c6d0902013 100644 --- a/src/List.h +++ b/src/List.h @@ -29,9 +29,9 @@ // TODO: this can be removed in v3.1 when List::sort() is removed typedef int (*list_cmp_func)(const void* v1, const void* v2); -enum list_order { LIST_ORDERED, LIST_UNORDERED }; +enum class ListOrder : int { ORDERED, UNORDERED }; -template +template class List { public: @@ -216,7 +216,7 @@ public: // For data where we don't care about ordering, we don't care about keeping // the list in the same order when removing an element. Just swap the last // element with the element being removed. - if ( Order == LIST_ORDERED ) + if constexpr ( Order == ListOrder::ORDERED ) { --num_entries; @@ -252,16 +252,16 @@ public: T replace(int ent_index, const T& new_ent) // replace entry #i with a new value { if ( ent_index < 0 ) - return get_default_value(); + return T{}; - T old_ent = get_default_value(); + T old_ent{}; if ( ent_index > num_entries - 1 ) { // replacement beyond the end of the list resize(ent_index + 1); for ( int i = num_entries; i < max_entries; ++i ) - entries[i] = get_default_value(); + entries[i] = T{}; num_entries = max_entries; } else @@ -299,20 +299,6 @@ public: protected: - T get_default_value() - { - if constexpr ( std::is_pointer_v ) - return nullptr; - else if constexpr ( std::is_integral_v ) - return 0; - else if constexpr ( std::is_floating_point_v ) - return 0.0; - else - // TODO: Should this return an error at compile time that we don't - // know what this type is? - return T(); - } - // This could essentially be an std::vector if we wanted. Some // reasons to maybe not refactor to use std::vector ? // @@ -344,7 +330,7 @@ protected: // Specialization of the List class to store pointers of a type. -template +template using PList = List; // Popular type of list: list of strings.