Coverage for libltdl/slist.c and fallout

[Ralf Wildenhues]

A first step at more libltdl coverage: better coverage for the slist
API.  Notes:

- I did find bugs in slist.c, albeit serious ones only in code not used
elsewhere in libltdl.

- slist_foreach and slist_find are redundant, as they have the same
semantics.

- slist_remove should IMVHO return an SList *, because otherwise there
is no way to avoid a memory leak.  APIs that force memleaks are bad.
(Apart from that, I've never really understood the difference between
boxed and unboxed stuff, basically you have to have a SList header in
order to put something in a list, period.  But I didn't want to mangle
the code beyond bug fixing really.)

- In the end I grew really lazy and added the new test to the old
testsuite: that seemed the easiest way to integrate and catch all the
compilation and include flags from toplevel Makefile.am in order to
build and use a separate slist.o object.  If this is not ok with you,
then please complain loudly.


So I have three patches I would like to commit as part of this.
The first one adds the test and fixes slist.c, the second one is only
stylistic and uses NULL instead of 0 throughout slist.c, and the third
one changes our public APIs lt_dlloader_remove and lt_dlloader_find to
accept `const char *' arguments instead of `char *': that's what fits
the purpose best, and what we document in the manual.  I'm not quite
sure whether the last one constitutes a compatible API change or only
a revision change, so versioning bump is still missing.

Review appreciated.

Thanks,
Ralf

    Test and fix slist.c.
    
    * libltdl/libltdl/slist.h: Include stddef.h, for size_t.
    (slist_remove): Return pointer to SList, not void.
    * libltdl/slist.c: Include stdlib.h, for malloc and free.
    (slist_remove): Adjust prototype as above.
    (slist_sort): Do not loop forever on one-item list.
    * tests/test-slist.c: New file.
    * Makefile.am (COMMON_TESTS): Add tests/test-slist.
    (check_PROGRAMS, tests_test_slist_SOURCES): New variables.

diff --git a/Makefile.am b/Makefile.am
index ecc54f8..a1097c7 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -672,6 +672,7 @@ COMMON_TESTS = \
        tests/sh.test \
        tests/suffix.test \
        tests/tagtrace.test \
+       tests/test-slist \
        tests/cdemo-static.test \
        tests/cdemo-static-make.test \
        tests/cdemo-static-exec.test \
@@ -759,6 +760,11 @@ COMMON_TESTS = \
        tests/cdemo-undef-make.test \
        tests/cdemo-undef-exec.test
 
+check_PROGRAMS = tests/test-slist
+tests_test_slist_SOURCES = \
+       tests/test-slist.c \
+       libltdl/slist.c
+
 tests/cdemo-undef-exec.log:    tests/cdemo-undef-make.log
 tests/cdemo-undef-make.log:    tests/cdemo-undef.log
 tests/cdemo-undef.log: @ORDER@ tests/cdemo-shared-exec.log
diff --git a/libltdl/libltdl/slist.h b/libltdl/libltdl/slist.h
index e4b7ead..4d56509 100644
--- a/libltdl/libltdl/slist.h
+++ b/libltdl/libltdl/slist.h
@@ -1,6 +1,6 @@
 /* slist.h -- generalised singly linked lists
 
-   Copyright (C) 2000, 2004 Free Software Foundation, Inc.
+   Copyright (C) 2000, 2004, 2009 Free Software Foundation, Inc.
    Written by Gary V. Vaughan, 2000
 
    NOTE: The canonical source of this file is maintained with the
@@ -48,6 +48,8 @@ or obtained by writing to the Free Software Foundation, Inc.,
 #  define LT_SCOPE
 #endif
 
+#include <stddef.h>
+
 #if defined(__cplusplus)
 extern "C" {
 #endif
@@ -65,7 +67,7 @@ LT_SCOPE SList *slist_concat  (SList *head, SList *tail);
 LT_SCOPE SList *slist_cons     (SList *item, SList *slist);
 
 LT_SCOPE SList *slist_delete   (SList *slist, void (*delete_fct) (void *item));
-LT_SCOPE void *        slist_remove    (SList **phead, SListCallback *find,
+LT_SCOPE SList *slist_remove   (SList **phead, SListCallback *find,
                                 void *matchdata);
 LT_SCOPE SList *slist_reverse  (SList *slist);
 LT_SCOPE SList *slist_sort     (SList *slist, SListCompare *compare,
diff --git a/libltdl/slist.c b/libltdl/slist.c
index c99f399..25906a4 100644
--- a/libltdl/slist.c
+++ b/libltdl/slist.c
@@ -1,6 +1,6 @@
 /* slist.c -- generalised singly linked lists
 
-   Copyright (C) 2000, 2004, 2007, 2008 Free Software Foundation, Inc.
+   Copyright (C) 2000, 2004, 2007, 2008, 2009 Free Software Foundation, Inc.
    Written by Gary V. Vaughan, 2000
 
    NOTE: The canonical source of this file is maintained with the
@@ -32,6 +32,7 @@ or obtained by writing to the Free Software Foundation, Inc.,
 
 #include "slist.h"
 #include <stddef.h>
+#include <stdlib.h>
 
 static SList * slist_sort_merge    (SList *left, SList *right,
                                     SListCompare *compare, void *userdata);
@@ -73,7 +74,7 @@ slist_delete (SList *head, void (*delete_fct) (void *item))
            the stale item, you should probably return that from FIND if
           it makes a successful match.  Don't forget to slist_unbox()
           every item in a boxed list before operating on its contents.   */
-void *
+SList *
 slist_remove (SList **phead, SListCallback *find, void *matchdata)
 {
   SList *stale = 0;
@@ -107,7 +108,7 @@ slist_remove (SList **phead, SListCallback *find, void 
*matchdata)
        }
     }
 
-  return result;
+  return (SList *) result;
 }
 
 /* Call FIND repeatedly with each element of SLIST and MATCHDATA, until
@@ -314,6 +315,9 @@ slist_sort (SList *slist, SListCompare *compare, void 
*userdata)
   left = slist;
   right = slist->next;
 
+  if (!right)
+    return left;
+
   /* Skip two items with RIGHT and one with SLIST, until RIGHT falls off
      the end.  SLIST must be about half way along.  */
   while (right && (right = right->next))
diff --git a/tests/test-slist.c b/tests/test-slist.c
new file mode 100644
index 0000000..40279a4
--- /dev/null
+++ b/tests/test-slist.c
@@ -0,0 +1,130 @@
+#include <config.h>
+#include <stdlib.h>
+#include <string.h>
+#include <assert.h>
+#include <stdio.h>
+#include "slist.h"
+
+void *find_string (SList *item, void *data)
+{
+  if (data != NULL && !strcmp ((const char *) item->userdata, (const char 
*)data))
+    return item;
+  else
+    return NULL;
+}
+
+void boxed_delete (void *item)
+{
+  free (slist_unbox ((SList *) item));
+}
+
+void *print_item (SList *item, void *userdata)
+{
+  userdata = userdata; /* unused */
+  printf ("%s\n", (const char*)item->userdata);
+  return NULL;
+}
+
+int list_compare (const SList *item1, const SList *item2, void *userdata)
+{
+  userdata = userdata;
+  return strcmp ((const char *) item1->userdata, (const char 
*)item2->userdata);
+}
+
+int main ()
+{
+  int i;
+  SList *empty_list = NULL, *list = NULL, *item, *list_save;
+  char *data = NULL;
+
+  /* slist_cons */
+  list = slist_cons (NULL, NULL);
+
+  for (i=0; i < 10; ++i) {
+    data = (char *) malloc (42);
+    assert (data);
+    sprintf (data, "foo%d", i);
+    list = slist_cons (slist_box (data), list);
+  }
+  list_save = list;
+  list = slist_cons (NULL, list);
+  assert (list == list_save);
+
+
+  /* slist_find */
+  assert (slist_find (NULL, find_string, (void *) "whatever") == NULL);
+  assert (slist_find (empty_list, find_string, (void *) "whatever") == NULL);
+  assert (slist_find (list, find_string, (void *) "foo10") == NULL);
+  item = (SList *) slist_find (list, find_string, (void *) "foo1");
+  assert (item != NULL);
+  assert (!strcmp ((const char *) item->userdata, "foo1"));
+
+  item = slist_nth (list, 10);
+  assert (item != NULL && !strcmp ((const char *) item->userdata, "foo0"));
+
+  puts ("list as inserted:");
+  slist_foreach (list, print_item, NULL);
+  puts ("reversed list:");
+  list = slist_reverse (list);
+  slist_foreach (list, print_item, NULL);
+
+  item = slist_nth (list, 1);
+  assert (item != NULL && !strcmp ((const char *) item->userdata, "foo0"));
+
+  assert (10 == slist_length (list));
+
+  /* slist_tail is the second item, not the last one */
+  item = slist_tail (list);
+  assert (item != NULL && !strcmp ((const char *) item->userdata, "foo1"));
+
+  assert (slist_tail (slist_nth (list, 10)) == NULL);
+
+  /* slist_sort and implicitly, slist_sort_merge */
+  assert (slist_sort (NULL, list_compare, NULL) == NULL);
+  list = slist_sort (list, list_compare, NULL);
+  puts ("list after no-op sort:");
+  slist_foreach (list, print_item, NULL);
+
+  list = slist_reverse (list);
+  puts ("reversed list:");
+  slist_foreach (list, print_item, NULL);
+  puts ("sorting reversed list:");
+  list = slist_sort (list, list_compare, NULL);
+  slist_foreach (list, print_item, NULL);
+
+  /* slist_remove */
+  assert (slist_remove (NULL, find_string, NULL) == NULL);
+  assert (slist_remove (&empty_list, find_string, NULL) == NULL);
+
+  list_save = list;
+  assert (slist_remove (&list, find_string, NULL) == NULL);
+  assert (list_save == list);
+
+  /* remove entries: middle, last, first, not present */
+  /* slist_reverse above has left us with increasing order */
+  list_save = list;
+  item = slist_remove (&list, find_string, (void *) "foo5");
+  assert (list_save == list);
+  assert (item != NULL && !strcmp (data = (char *) slist_unbox (item), 
"foo5"));
+  free (data);
+
+  list_save = list;
+  item = slist_remove (&list, find_string, (void *) "foo9");
+  assert (list_save == list);
+  assert (item != NULL && !strcmp (data = (char *) slist_unbox (item), 
"foo9"));
+  free (data);
+
+  list_save = list;
+  item = slist_remove (&list, find_string, (void *) "foo0");
+  assert (list_save != list);
+  assert (item != NULL && !strcmp (data = (char *) slist_unbox (item), 
"foo0"));
+  free (data);
+
+  list_save = list;
+  item = slist_remove (&list, find_string, (void *) "foo5");
+  assert (list_save == list);
+  assert (item == NULL);
+
+  assert (slist_delete (list, boxed_delete) == NULL);
+  return 0;
+}




    Use NULL instead of 0 in slist.c.
    
    * libltdl/slist.c (slist_delete, slist_remove, slist_find)
    (slist_reverse, slist_foreach, slist_sort, slist_box)
    (slist_unbox): Use NULL for pointers throughout.

diff --git a/libltdl/slist.c b/libltdl/slist.c
index 25906a4..07a2e10 100644
--- a/libltdl/slist.c
+++ b/libltdl/slist.c
@@ -62,7 +62,7 @@ slist_delete (SList *head, void (*delete_fct) (void *item))
       head = next;
     }
 
-  return 0;
+  return NULL;
 }
 
 /* Call FIND repeatedly with MATCHDATA and each item of *PHEAD, until
@@ -77,13 +77,13 @@ slist_delete (SList *head, void (*delete_fct) (void *item))
 SList *
 slist_remove (SList **phead, SListCallback *find, void *matchdata)
 {
-  SList *stale = 0;
-  void *result = 0;
+  SList *stale = NULL;
+  void *result = NULL;
 
   assert (find);
 
   if (!phead || !*phead)
-    return 0;
+    return NULL;
 
   /* Does the head of the passed list match? */
   result = (*find) (*phead, matchdata);
@@ -117,7 +117,7 @@ slist_remove (SList **phead, SListCallback *find, void 
*matchdata)
 void *
 slist_find (SList *slist, SListCallback *find, void *matchdata)
 {
-  void *result = 0;
+  void *result = NULL;
 
   assert (find);
 
@@ -222,7 +222,7 @@ slist_length (SList *slist)
 SList *
 slist_reverse (SList *slist)
 {
-  SList *result = 0;
+  SList *result = NULL;
   SList *next;
 
   while (slist)
@@ -241,7 +241,7 @@ slist_reverse (SList *slist)
 void *
 slist_foreach (SList *slist, SListCallback *foreach, void *userdata)
 {
-  void *result = 0;
+  void *result = NULL;
 
   assert (foreach);
 
@@ -327,7 +327,7 @@ slist_sort (SList *slist, SListCompare *compare, void 
*userdata)
       slist = slist->next;
     }
   right = slist->next;
-  slist->next = 0;
+  slist->next = NULL;
 
   /* Sort LEFT and RIGHT, then merge the two.  */
   return slist_sort_merge (slist_sort (left, compare, userdata),
@@ -354,7 +354,7 @@ slist_box (const void *userdata)
 
   if (item)
     {
-      item->next     = 0;
+      item->next     = NULL;
       item->userdata = userdata;
     }
 
@@ -365,7 +365,7 @@ slist_box (const void *userdata)
 void *
 slist_unbox (SList *item)
 {
-  void *userdata = 0;
+  void *userdata = NULL;
 
   if (item)
     {



    lt_dlloader_remove and lt_dlloader_find accept const arguments.
    
    * libltdl/lt_dlloader.c (lt_dlloader_remove, lt_dlloader_find):
    Accept `const char *' arguments, as documented.  Cast them to
    `void *' for the slist machinery.
    * libltdl/libltdl/lt_dlloader.h: Adjust prototypes.

diff --git a/libltdl/libltdl/lt_dlloader.h b/libltdl/libltdl/lt_dlloader.h
index ae131fa..589fd0d 100644
--- a/libltdl/libltdl/lt_dlloader.h
+++ b/libltdl/libltdl/lt_dlloader.h
@@ -73,8 +73,8 @@ typedef struct {
 LT_SCOPE int           lt_dlloader_add    (const lt_dlvtable *vtable);
 LT_SCOPE lt_dlloader   lt_dlloader_next   (const lt_dlloader loader);
 
-LT_SCOPE lt_dlvtable * lt_dlloader_remove      (char *name);
-LT_SCOPE const lt_dlvtable *lt_dlloader_find   (char *name);
+LT_SCOPE lt_dlvtable * lt_dlloader_remove      (const char *name);
+LT_SCOPE const lt_dlvtable *lt_dlloader_find   (const char *name);
 LT_SCOPE const lt_dlvtable *lt_dlloader_get    (lt_dlloader loader);
 
 
diff --git a/libltdl/lt_dlloader.c b/libltdl/lt_dlloader.c
index 4e66a6c..2c99a22 100644
--- a/libltdl/lt_dlloader.c
+++ b/libltdl/lt_dlloader.c
@@ -150,7 +150,7 @@ lt_dlloader_get     (lt_dlloader loader)
    modules need this loader; in either case, the loader list is not
    changed if NULL is returned.  */
 lt_dlvtable *
-lt_dlloader_remove (char *name)
+lt_dlloader_remove (const char *name)
 {
   const lt_dlvtable *  vtable  = lt_dlloader_find (name);
   static const char    id_string[] = "lt_dlloader_remove";
@@ -199,12 +199,12 @@ lt_dlloader_remove (char *name)
 
   /* If we got this far, remove the loader from our global list.  */
   return (lt_dlvtable *)
-      slist_unbox ((SList *) slist_remove (&loaders, loader_callback, name));
+      slist_unbox ((SList *) slist_remove (&loaders, loader_callback, (void *) 
name));
 }
 
 
 const lt_dlvtable *
-lt_dlloader_find (char *name)
+lt_dlloader_find (const char *name)
 {
-  return lt_dlloader_get (slist_find (loaders, loader_callback, name));
+  return lt_dlloader_get (slist_find (loaders, loader_callback, (void *) 
name));
 }