Bug in scm_reverse (including patch)

[Dirk Herrmann <dirk at ida dot ing dot tu-bs dot de>]

Hi!

The code for scm_reverse is buggy:

guile> (reverse '(1 2 . a))

Backtrace:
0* [reverse (1 2 . a)]

ERROR: In procedure reverse in expression (reverse (quote #)):
ERROR: Bad memory access (Segmentation violation)
ABORT: (signal)

Here's a patch, which also get's rid of another couple of calls to 
SCM_NIMP, places the documentation for reverse! at the correct place,
renames C parameter names to match the names in the documentation strings,
tries to unify the tortoise and hare algorithm in scm_reverse with the one
in scm_ilength, get's rid of unnecessary 'register' keywords (let the
compiler do register allocation) and minimizes the scope of the 'old_tail'
variable in function scm_reverse_x.

After the patch is applied you get the expected result:

guile> (reverse '(1 2 . a))

Backtrace:
0* [reverse (1 2 . a)]

ERROR: In procedure reverse in expression (reverse (quote #)):
ERROR: Wrong type argument in position 1: (1 2 . a)
ABORT: (wrong-type-arg)


===================================================================
RCS file: /cvs/guile/guile/guile-core/libguile/list.c,v
retrieving revision 1.23
diff -u -p -r1.23 list.c
--- list.c      2000/01/05 19:25:36     1.23
+++ list.c      2000/01/06 14:05:02
@@ -141,16 +141,16 @@ SCM_DEFINE (scm_list_p, "list?", 1, 0, 0
 long
 scm_ilength(SCM sx)
 {
-  register long i = 0;
-  register SCM tortoise = sx;
-  register SCM hare = sx;
+  long i = 0;
+  SCM tortoise = sx;
+  SCM hare = sx;
 
   do {
-    if (SCM_IMP(hare)) return SCM_NULLP(hare) ? i : -1;
+    if (SCM_NULLP(hare)) return i;
     if (SCM_NCONSP(hare)) return -1;
     hare = SCM_CDR(hare);
     i++;
-    if (SCM_IMP(hare)) return SCM_NULLP(hare) ? i : -1;
+    if (SCM_NULLP(hare)) return i;
     if (SCM_NCONSP(hare)) return -1;
     hare = SCM_CDR(hare);
     i++;
@@ -203,8 +203,7 @@ performed.  Return a pointer to the muta
       return res;
     }
     SCM_VALIDATE_CONS (SCM_ARGn, args);
-    for(;SCM_NIMP(arg);arg = SCM_CDR(arg)) {
-      SCM_VALIDATE_CONS (SCM_ARGn, arg);
+    for (; SCM_CONSP(arg); arg = SCM_CDR(arg)) {
       *lloc = scm_cons(SCM_CAR(arg), SCM_EOL);
       lloc = SCM_CDRLOC(*lloc);
     }
@@ -264,6 +263,31 @@ SCM_DEFINE (scm_last_pair, "last-pair", 
 
 SCM_DEFINE (scm_reverse, "reverse", 1, 0, 0,
             (SCM ls),
+"")
+#define FUNC_NAME s_scm_reverse
+{
+  SCM result = SCM_EOL;
+  SCM tortoise = ls;
+  SCM hare = ls;
+
+  do {
+      if (SCM_NULLP(hare)) return result;
+      SCM_ASSERT(SCM_CONSP(hare), lst, 1, FUNC_NAME);
+      result = scm_cons (SCM_CAR (hare), result);
+      hare = SCM_CDR (hare);
+      if (SCM_NULLP(hare)) return result;
+      SCM_ASSERT(SCM_CONSP(hare), lst, 1, FUNC_NAME);
+      result = scm_cons (SCM_CAR (hare), result);
+      hare = SCM_CDR (hare);
+      tortoise = SCM_CDR (tortoise);
+    }
+  while (hare != tortoise);
+  scm_misc_error (FUNC_NAME, "Circular structure: %S", SCM_LIST1 (ls));
+}
+#undef FUNC_NAME
+
+SCM_DEFINE (scm_reverse_x, "reverse!", 1, 1, 0,
+            (SCM ls, SCM new_tail),
 "A destructive version of @code{reverse} (@pxref{Pairs and Lists,,,r4rs,
 The Revised^4 Report on Scheme}).  The cdr of each cell in @var{lst} is
 modified to point to the previous list element.  Return a pointer to the
@@ -275,36 +299,8 @@ the tail.  Therefore, the @var{lst} symb
 original list was bound now points to the tail.  To ensure that the head
 of the modified list is not lost, it is wise to save the return value of
 @code{reverse!}")
-#define FUNC_NAME s_scm_reverse
-{
-  SCM res = SCM_EOL;
-  SCM p = ls, t = ls;
-  while (SCM_NIMP (p))
-    {
-      SCM_VALIDATE_CONS (1,ls);
-      res = scm_cons (SCM_CAR (p), res);
-      p = SCM_CDR (p);
-      if (SCM_IMP (p))
-       break;
-      SCM_VALIDATE_CONS (1,ls);
-      res = scm_cons (SCM_CAR (p), res);
-      p = SCM_CDR (p);
-      t = SCM_CDR (t);
-      if (t == p)
-       scm_misc_error (FUNC_NAME, "Circular structure: %S", SCM_LIST1 (ls));
-    }
-  ls = p;
-  SCM_VALIDATE_NULL (1,ls);
-  return res;
-}
-#undef FUNC_NAME
-
-SCM_DEFINE (scm_reverse_x, "reverse!", 1, 1, 0,
-            (SCM ls, SCM new_tail),
-"")
 #define FUNC_NAME s_scm_reverse_x
 {
-  SCM old_tail;
   SCM_ASSERT (scm_ilength (ls) >= 0, ls, SCM_ARG1, FUNC_NAME);
   if (SCM_UNBNDP (new_tail))
     new_tail = SCM_EOL;
@@ -313,7 +309,7 @@ SCM_DEFINE (scm_reverse_x, "reverse!", 1
 
   while (SCM_NIMP (ls))
     {
-      old_tail = SCM_CDR (ls);
+      SCM old_tail = SCM_CDR (ls);
       SCM_SETCDR (ls, new_tail);
       new_tail = ls;
       ls = old_tail;