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));
}
