Re: compiling with -DSCM_DEBUG=1

[Ken Raeburn]

On Nov 14, 2009, at 12:07, Ken Raeburn wrote:
> On Nov 14, 2009, at 08:47, Neil Jerram wrote:
> > Thanks, Andy.  I'm confident now that your patch is correct, Ken, so
> > please could you apply it?  Also please let me know if you're happy  
> > to
> > do the other changes (mostly comment updates) that I suggested to go
> > with it, or if you'd prefer me to do those.
>
> Sure, I'll look at updating the comments too.  Based on my current  
> understanding, though, I'm seeing more code updates that might be  
> desirable, like not checking SCM_CLASSP(SCM_CAR(z)) etc, if a non- 
> pair is the one and only indication of hitting the end of the  
> specifiers.

Here's my revised patch.  I've simplified the check, and it still  
passes the tests (except the options tests that were just committed  
with a log message indicating that they don't pass) and doesn't crash  
under SCM_DEBUG.  More details in the comments might be nice, I think,  
but I'm not familiar enough with that part of the code to fill them  
in, and I'd rather get back to my Emacs hacking than spend a lot more  
time on that right now.

Answering Ludo's question from Oct 31 that I meant to get back to  
sooner: No, I don't have a simple test case yet, other than building  
the tree and letting the compiler run; I'm not sure yet what needs to  
be done exactly to trigger the bug.

    SCM_DEBUG fix: Don't apply SCM_CAR to non-pairs when walking  
argument
    lists in method cache matching.

    * libguile/eval.i.c (CEVAL): Don't apply SCM_CAR to non-pairs when
      walking argument lists in method cache matching.  Don't check for
      CLASSP or symbol in the car slot, since the end of the specifier
      list is a non-pair.
    * libguile/objects.c (scm_mcache_lookup_cmethod): Likewise.  Update
      comments to reflect new structure of method cache entry.
    * module/oops/goops/dispatch.scm: Update comments here too.

diff --git a/libguile/eval.i.c b/libguile/eval.i.c
index 5b4604a..dffbe9f 100644
--- a/libguile/eval.i.c
+++ b/libguile/eval.i.c
@@ -839,21 +839,19 @@ dispatch:
                {
                  SCM args = arg1; /* list of arguments */
                  z = SCM_SIMPLE_VECTOR_REF (method_cache, hash_value);
-                 while (!scm_is_null (args))
+                 /* More arguments than specifiers => z = CMETHOD, not a pair.
+                  * Fewer arguments than specifiers => CAR != CLASS or
+                  * `no-method'.  */
+                 while (scm_is_pair (z) && !scm_is_null (args))
                    {
-                     /* More arguments than specifiers => CLASS != ENV */
                      SCM class_of_arg = scm_class_of (SCM_CAR (args));
                      if (!scm_is_eq (class_of_arg, SCM_CAR (z)))
                        goto next_method;
                      args = SCM_CDR (args);
                      z = SCM_CDR (z);
                    }
-                 /* Fewer arguments than specifiers => CAR != CLASS */
                   if (!scm_is_pair (z))
                     goto apply_vm_cmethod;
-                  else if (!SCM_CLASSP (SCM_CAR (z))
-                           && !scm_is_symbol (SCM_CAR (z)))
-                    goto apply_memoized_cmethod;
                next_method:
                  hash_value = (hash_value + 1) & mask;
                } while (hash_value != cache_end_pos);
diff --git a/libguile/objects.c b/libguile/objects.c
index f686c3a..b05c9c7 100644
--- a/libguile/objects.c
+++ b/libguile/objects.c
@@ -57,12 +57,12 @@ SCM scm_metaclass_operator;
  *
  * Format #1:
  * (SCM_IM_DISPATCH ARGS N-SPECIALIZED
- *   #((TYPE1 ... ENV FORMALS FORM ...) ...)
+ *   #((TYPE1 ... . CMETHOD) ...)
  *   GF)
  *
  * Format #2:
  * (SCM_IM_HASH_DISPATCH ARGS N-SPECIALIZED HASHSET MASK
- *   #((TYPE1 ... ENV FORMALS FORM ...) ...)
+ *   #((TYPE1 ... . CMETHOD) ...)
  *   GF)
  *
  * ARGS is either a list of expressions, in which case they
@@ -73,9 +73,6 @@ SCM scm_metaclass_operator;
  * SCM_IM_DISPATCH expressions in generic functions always
  * have ARGS = the symbol `args' or the iloc address@hidden
  *
- * Need FORMALS in order to support varying arity.  This
- * also avoids the need for renaming of bindings.
- *
  * We should probably not complicate this mechanism by
  * introducing "optimizations" for getters and setters or
  * primitive methods.  Getters and setter will normally be
@@ -131,19 +128,18 @@ scm_mcache_lookup_cmethod (SCM cache, SCM args)
       long j = n;
       z = SCM_SIMPLE_VECTOR_REF (methods, i);
       ls = args; /* list of arguments */
-      if (!scm_is_null (ls))
+      /* More arguments than specifiers => z = CMETHOD, not a pair.
+       * Fewer arguments than specifiers => CAR != CLASS or `no- 
method'.  */
+      if (!scm_is_null (ls) && scm_is_pair (z))
        do
          {
-           /* More arguments than specifiers => CLASS != ENV */
            if (! scm_is_eq (scm_class_of (SCM_CAR (ls)), SCM_CAR (z)))
              goto next_method;
            ls = SCM_CDR (ls);
            z = SCM_CDR (z);
          }
-       while (j-- && !scm_is_null (ls));
-      /* Fewer arguments than specifiers => CAR != CLASS or `no- 
method' */
-      if (!scm_is_pair (z)
-          || (!SCM_CLASSP (SCM_CAR (z)) && !scm_is_symbol (SCM_CAR  
(z))))
+       while (j-- && !scm_is_null (ls) && scm_is_pair (z));
+      if (!scm_is_pair (z))
        return z;
     next_method:
       i = (i + 1) & mask;
diff --git a/module/oop/goops/dispatch.scm b/module/oop/goops/ 
dispatch.scm
index 88abf80..6a450c1 100644
--- a/module/oop/goops/dispatch.scm
+++ b/module/oop/goops/dispatch.scm
@@ -53,9 +53,9 @@
 ;;; Method cache
 ;;;

-;; (address@hidden args N-SPECIALIZED #((TYPE1 ... ENV FORMALS  
FORM1 ...) ...) GF)
+;; (address@hidden args N-SPECIALIZED #((TYPE1 ... . CMETHOD) ...) GF)
 ;; (address@hidden args N-SPECIALIZED HASHSET MASK
-;;             #((TYPE1 ... ENV FORMALS FORM1 ...) ...)
+;;             #((TYPE1 ... . CMETHOD) ...)
 ;;             GF)

 ;;; Representation