linked-list: Order of insert_before/remove_at calls doesn't matter anymore

This was quite confusing previously:  While calling insert_before()
and then remove_at() properly replaced the current item, calling them the
other way around inserted the new item before the previous item because
remove_at() changed the enumerator's position to the previous item.

The behavior in corner cases (calling the methods before or after
enumeration) is also changed slightly.

diff --git a/src/libstrongswan/collections/linked_list.c b/src/libstrongswan/collections/linked_list.c
index 5ad7360d6f295a335fa2b7786432cfb989ec1321..c7342c6d6c409c30597a3abee81dd0476636dd2f 100644 (file)
--- a/src/libstrongswan/collections/linked_list.c
+++ b/src/libstrongswan/collections/linked_list.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2007-2015 Tobias Brunner
+ * Copyright (C) 2007-2018 Tobias Brunner
  * Copyright (C) 2005-2006 Martin Willi
  * Copyright (C) 2005 Jan Hutter
  * HSR Hochschule fuer Technik Rapperswil
@@ -111,7 +111,7 @@ struct private_enumerator_t {
        /**
         * implements enumerator interface
         */
-       enumerator_t enumerator;
+       enumerator_t public;
 
        /**
         * associated linked list
@@ -122,35 +122,19 @@ struct private_enumerator_t {
         * current item
         */
        element_t *current;
-
-       /**
-        * enumerator has enumerated all items
-        */
-       bool finished;
 };
 
-METHOD(enumerator_t, enumerate, bool,
-       private_enumerator_t *this, va_list args)
+/**
+ * Enumerate the current item
+ */
+static bool do_enumerate(private_enumerator_t *this, va_list args)
 {
        void **item;
 
        VA_ARGS_VGET(args, item);
 
-       if (this->finished)
-       {
-               return FALSE;
-       }
        if (!this->current)
        {
-               this->current = this->list->first;
-       }
-       else
-       {
-               this->current = this->current->next;
-       }
-       if (!this->current)
-       {
-               this->finished = TRUE;
                return FALSE;
        }
        if (item)
@@ -160,28 +144,46 @@ METHOD(enumerator_t, enumerate, bool,
        return TRUE;
 }
 
+METHOD(enumerator_t, enumerate_next, bool,
+       private_enumerator_t *this, va_list args)
+{
+       if (this->current)
+       {
+               this->current = this->current->next;
+       }
+       return do_enumerate(this, args);
+}
+
+METHOD(enumerator_t, enumerate_current, bool,
+       private_enumerator_t *this, va_list args)
+{
+       this->public.venumerate = _enumerate_next;
+       return do_enumerate(this, args);
+}
+
 METHOD(linked_list_t, create_enumerator, enumerator_t*,
        private_linked_list_t *this)
 {
        private_enumerator_t *enumerator;
 
        INIT(enumerator,
-               .enumerator = {
+               .public = {
                        .enumerate = enumerator_enumerate_default,
-                       .venumerate = _enumerate,
+                       .venumerate = _enumerate_current,
                        .destroy = (void*)free,
                },
                .list = this,
+               .current = this->first,
        );
 
-       return &enumerator->enumerator;
+       return &enumerator->public;
 }
 
 METHOD(linked_list_t, reset_enumerator, void,
        private_linked_list_t *this, private_enumerator_t *enumerator)
 {
-       enumerator->current = NULL;
-       enumerator->finished = FALSE;
+       enumerator->current = this->first;
+       enumerator->public.venumerate = _enumerate_current;
 }
 
 METHOD(linked_list_t, get_count, int,
@@ -298,14 +300,7 @@ METHOD(linked_list_t, insert_before, void,
        current = enumerator->current;
        if (!current)
        {
-               if (enumerator->finished)
-               {
-                       this->public.insert_last(&this->public, item);
-               }
-               else
-               {
-                       this->public.insert_first(&this->public, item);
-               }
+               insert_last(this, item);
                return;
        }
        element = element_create(item);
@@ -377,7 +372,9 @@ METHOD(linked_list_t, remove_at, void,
        if (enumerator->current)
        {
                current = enumerator->current;
-               enumerator->current = current->previous;
+               enumerator->current = current->next;
+               /* the enumerator already points to the next item */
+               enumerator->public.venumerate = _enumerate_current;
                remove_element(this, current);
        }
 }

diff --git a/src/libstrongswan/collections/linked_list.h b/src/libstrongswan/collections/linked_list.h
index a9cb7f0d455292a5fe5117d65229e340ea7cf0e5..315fb05209e2d0177deb54fe88fc7873808de8e1 100644 (file)
--- a/src/libstrongswan/collections/linked_list.h
+++ b/src/libstrongswan/collections/linked_list.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2007-2017 Tobias Brunner
+ * Copyright (C) 2007-2018 Tobias Brunner
  * Copyright (C) 2005-2008 Martin Willi
  * Copyright (C) 2005 Jan Hutter
  * HSR Hochschule fuer Technik Rapperswil
@@ -102,12 +102,17 @@ struct linked_list_t {
        /**
         * Inserts a new item before the item the enumerator currently points to.
         *
-        * If this method is called before starting the enumeration the item is
-        * inserted first. If it is called after all items have been enumerated
-        * the item is inserted last. This is helpful when inserting items into
-        * a sorted list.
+        * If this method is called after all items have been enumerated, the item
+        * is inserted last.  This is helpful when inserting items into a sorted
+        * list.
         *
-        * @note The position of the enumerator is not changed.
+        * @note The position of the enumerator is not changed. So it is safe to
+        * call this before or after remove_at() to replace the item at the current
+        * position (the enumerator will continue with the next item in the list).
+        * And in particular, when inserting an item before calling enumerate(),
+        * the enumeration will continue (or start) at the item that was first in
+        * the list before any items were inserted (enumerate() will return FALSE
+        * if the list was empty before).
         *
         * @param enumerator    enumerator with position
         * @param item                  item value to insert in list
@@ -118,6 +123,10 @@ struct linked_list_t {
        /**
         * Remove an item from the list where the enumerator points to.
         *
+        * If this method is called before calling enumerate() of the enumerator,
+        * the first item in the list, if any, will be removed.  No item is removed,
+        * if the method is called after enumerating all items.
+        *
         * @param enumerator enumerator with position
         */
        void (*remove_at)(linked_list_t *this, enumerator_t *enumerator);

diff --git a/src/libstrongswan/tests/suites/test_linked_list_enumerator.c b/src/libstrongswan/tests/suites/test_linked_list_enumerator.c
index 19f381ef3ace7347cffbe774182498919d47f398..30b7b5c118b1cc0617ad422aa7b3eb16633add29 100644 (file)
--- a/src/libstrongswan/tests/suites/test_linked_list_enumerator.c
+++ b/src/libstrongswan/tests/suites/test_linked_list_enumerator.c
@@ -144,11 +144,12 @@ START_TEST(test_insert_before_ends)
        int round;
 
        enumerator = list->create_enumerator(list);
+       /* this does not change the enumerator position, which points to 1 */
        list->insert_before(list, enumerator, (void*)0);
        ck_assert_int_eq(list->get_count(list), 6);
        ck_assert(list->get_first(list, (void*)&x) == SUCCESS);
        ck_assert_int_eq(x, 0);
-       round = 0;
+       round = 1;
        while (enumerator->enumerate(enumerator, &x))
        {
                ck_assert_int_eq(round, x);
@@ -177,8 +178,13 @@ START_TEST(test_insert_before_empty)
        ck_assert_int_eq(x, 1);
        ck_assert(list->get_last(list, (void*)&x) == SUCCESS);
        ck_assert_int_eq(x, 1);
-       ck_assert(enumerator->enumerate(enumerator, &x));
+       ck_assert(!enumerator->enumerate(enumerator, &x));
+       list->insert_before(list, enumerator, (void*)2);
+       ck_assert_int_eq(list->get_count(list), 2);
+       ck_assert(list->get_first(list, (void*)&x) == SUCCESS);
        ck_assert_int_eq(x, 1);
+       ck_assert(list->get_last(list, (void*)&x) == SUCCESS);
+       ck_assert_int_eq(x, 2);
        ck_assert(!enumerator->enumerate(enumerator, NULL));
        enumerator->destroy(enumerator);
 }
@@ -221,6 +227,43 @@ START_TEST(test_remove_at)
 }
 END_TEST
 
+START_TEST(test_remove_at_multi)
+{
+       enumerator_t *enumerator;
+       intptr_t x;
+       int round;
+
+       round = 1;
+       enumerator = list->create_enumerator(list);
+       while (enumerator->enumerate(enumerator, &x))
+       {
+               ck_assert_int_eq(round, x);
+               if (round == 2 || round == 5)
+               {
+                       list->remove_at(list, enumerator);
+               }
+               round++;
+       }
+       ck_assert_int_eq(list->get_count(list), 3);
+       list->reset_enumerator(list, enumerator);
+       round = 1;
+       while (enumerator->enumerate(enumerator, &x))
+       {
+               if (round == 2)
+               {       /* skip removed item */
+                       round++;
+               }
+               ck_assert_int_eq(round, x);
+               list->remove_at(list, enumerator);
+               round++;
+       }
+       ck_assert_int_eq(list->get_count(list), 0);
+       list->reset_enumerator(list, enumerator);
+       ck_assert(!enumerator->enumerate(enumerator, &x));
+       enumerator->destroy(enumerator);
+}
+END_TEST
+
 START_TEST(test_remove_at_ends)
 {
        enumerator_t *enumerator;
@@ -228,14 +271,14 @@ START_TEST(test_remove_at_ends)
 
        enumerator = list->create_enumerator(list);
        list->remove_at(list, enumerator);
-       ck_assert_int_eq(list->get_count(list), 5);
+       ck_assert_int_eq(list->get_count(list), 4);
        ck_assert(list->get_first(list, (void*)&x) == SUCCESS);
-       ck_assert_int_eq(x, 1);
+       ck_assert_int_eq(x, 2);
        while (enumerator->enumerate(enumerator, &x))
        {
        }
        list->remove_at(list, enumerator);
-       ck_assert_int_eq(list->get_count(list), 5);
+       ck_assert_int_eq(list->get_count(list), 4);
        ck_assert(list->get_last(list, (void*)&x) == SUCCESS);
        ck_assert_int_eq(x, 5);
        enumerator->destroy(enumerator);
@@ -254,14 +297,12 @@ START_TEST(test_insert_before_remove_at)
        {
                ck_assert_int_eq(round, x);
                if (round == 2)
-               {       /* this replaces the current item, as insert_before does not change
-                        * the enumerator position */
+               {       /* this replaces the current item */
                        list->insert_before(list, enumerator, (void*)42);
                        list->remove_at(list, enumerator);
                }
                else if (round == 4)
-               {       /* this does not replace the item, as remove_at moves the enumerator
-                        * position to the previous item */
+               {       /* same here, the order of calls does not matter */
                        list->remove_at(list, enumerator);
                        list->insert_before(list, enumerator, (void*)21);
                }
@@ -276,13 +317,9 @@ START_TEST(test_insert_before_remove_at)
                {       /* check replaced item */
                        ck_assert_int_eq(42, x);
                }
-               else if (round == 3)
-               {       /* check misplaced item */
-                       ck_assert_int_eq(21, x);
-               }
                else if (round == 4)
-               {       /* check misplaced item */
-                       ck_assert_int_eq(3, x);
+               {       /* check replace item */
+                       ck_assert_int_eq(21, x);
                }
                else
                {
@@ -348,6 +385,7 @@ Suite *linked_list_enumerator_suite_create()
        tc = tcase_create("modify");
        tcase_add_checked_fixture(tc, setup_list, teardown_list);
        tcase_add_test(tc, test_remove_at);
+       tcase_add_test(tc, test_remove_at_multi);
        tcase_add_test(tc, test_remove_at_ends);
        tcase_add_test(tc, test_insert_before_remove_at);
        suite_add_tcase(s, tc);