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 #...@0-0.
*
- * 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
;;;
-;; (#...@dispatch args N-SPECIALIZED #((TYPE1 ... ENV FORMALS
FORM1 ...) ...) GF)
+;; (#...@dispatch args N-SPECIALIZED #((TYPE1 ... . CMETHOD) ...) GF)
;; (#...@dispatch args N-SPECIALIZED HASHSET MASK
-;; #((TYPE1 ... ENV FORMALS FORM1 ...) ...)
+;; #((TYPE1 ... . CMETHOD) ...)
;; GF)
;;; Representation
