Merge remote-tracking branch 'origin/topic/timw/unordered-lists'

* origin/topic/timw/unordered-lists:
  Review fixups
  Change timer_list in BroList to be an unordered list.
  Add ability for List to be ordered/unordered
  Add unit testing for List
  Fix bug with List where replace() doesn't work with non-pointer types

CHANGES

@@ -1,4 +1,22 @@
 
+3.2.0-dev.484 | 2020-05-08 11:50:54 -0700
+
+  * Change timer_list in BroList to be an unordered list. (Tim Wojtulewicz, Corelight)
+
+    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.
+
+  * Add ability for List to be ordered/unordered (Tim Wojtulewicz, Corelight)
+
+    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.
+
+  * Add unit testing for List (Tim Wojtulewicz, Corelight)
+
+  * Fix bug with List where replace() doesn't work with non-pointer types (Tim Wojtulewicz, Corelight)
+
 3.2.0-dev.478 | 2020-05-08 11:47:38 -0700
 
   * Added examples to set_to_regex comments (James Lagermann, Corelight)

VERSION

@@ -1 +1 @@
-3.2.0-dev.478
+3.2.0-dev.484

src/BroList.h

@@ -23,4 +23,4 @@ class Attr;
 typedef PList<Attr> attr_list;
 
 class Timer;
-typedef PList<Timer> timer_list;
+typedef PList<Timer, ListOrder::UNORDERED> timer_list;

src/CMakeLists.txt

@@ -246,6 +246,7 @@ set(MAIN_SRCS
     IntSet.cc
     IP.cc
     IPAddr.cc
+    List.cc
     Reporter.cc
     NFA.cc
     Net.cc

src/List.cc

@@ -0,0 +1,130 @@
+#include <List.h>
+#include <3rdparty/doctest.h>
+
+TEST_CASE("list construction")
+	{
+	List<int> list;
+	CHECK(list.empty());
+
+	List<int> list2(10);
+	CHECK(list2.empty());
+	CHECK(list2.max() == 10);
+	}
+
+TEST_CASE("list operation")
+	{
+	List<int> 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<int> 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<int> 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();
+	}
+
+TEST_CASE("unordered list operation")
+	{
+	List<int, ListOrder::UNORDERED> 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);
+	}

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<typename T>
+enum class ListOrder : int { ORDERED, UNORDERED };
+
+template<typename T, ListOrder Order = ListOrder::ORDERED>
 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 constexpr ( Order == ListOrder::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;
 		}
@@ -240,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 0;
+			return T{};
 
-		T old_ent = nullptr;
+		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] = nullptr;
+				entries[i] = T{};
 			num_entries = max_entries;
 			}
 		else
@@ -318,8 +330,8 @@ protected:
 
 
 // Specialization of the List class to store pointers of a type.
-template<typename T>
-using PList = List<T*>;
+template<typename T, ListOrder Order = ListOrder::ORDERED>
+using PList = List<T*, Order>;
 
 // Popular type of list: list of strings.
 typedef PList<char> name_list;