Re: slowness in guile 1.8

[Ludovic Courtès]

Hi,

address@hidden (Ludovic Courtès) writes:

> Actually, no: the test does a `define' _within_ the body of `begin', so
> I *think* this qualifies as an internal define, and internal defines are
> equivalent to `letrec' (Section 5.2.2).

I was wrong: `define' within `begin' does not qualify as an "internal
define", so the test "binding is created before expression is evaluated"
was incorrect (or over-specified compared to R5RS[*]).

After further discussion with Andy, I committed the patch below to HEAD.
It inverts the order of expression evaluation and `scm_sym2var' in
`scm_m_define ()' (which is concerned only with top-level defines),
thereby fixing the `(define round round)' case.  At the same time, it
breaks the aforementioned test from `syntax.test', but there's nothing
wrong with that.

For 1.8, I'm pretty much inclined to commit a similar patch, i.e., where
`module-make-local-var!' and `scm_m_define' are copied from HEAD.  This
would break code that does things like:

  (define foo (begin (set! foo 1) (+ foo 1)))

but I think it's reasonable to break such code (which relies on
non-R5RS-compliant behavior anyway), especially given the performance
gain we get in return.  What do you think?

Thanks,
Ludovic.

[*] FWIW, the wording for `define' in the newly-released R5.93RS
    (Section 9.3.1) is the same as that of R5RS (Section 5.2.1).


--- orig/libguile/ChangeLog
+++ mod/libguile/ChangeLog
@@ -1,3 +1,9 @@
+2007-05-26  Ludovic Courtès  <address@hidden>
+
+       * eval.c (scm_m_define): Updated comment.  Changed order for value
+       evaluation and `scm_sym2var ()' call, which is perfectly valid per
+       R5RS.  This reverts the change dated 2004-04-22 by Dirk Herrmann.
+
 2007-05-05  Ludovic Courtès  <address@hidden>
 
        Implemented lazy duplicate binding handling.


--- orig/libguile/eval.c
+++ mod/libguile/eval.c
@@ -1209,10 +1209,11 @@
   return expr;
 }
 
-/* According to section 5.2.1 of R5RS we first have to make sure that the
- * variable is bound, and then perform the (set! variable expression)
- * operation.  This means, that within the expression we may already assign
- * values to variable: (define foo (begin (set! foo 1) (+ foo 1)))  */
+/* According to Section 5.2.1 of R5RS we first have to make sure that the
+   variable is bound, and then perform the `(set! variable expression)'
+   operation.  However, EXPRESSION _can_ be evaluated before VARIABLE is
+   bound.  This means that EXPRESSION won't necessarily be able to assign
+   values to VARIABLE as in `(define foo (begin (set! foo 1) (+ foo 1)))'.  */
 SCM
 scm_m_define (SCM expr, SCM env)
 {
@@ -1222,9 +1223,9 @@
     const SCM canonical_definition = canonicalize_define (expr);
     const SCM cdr_canonical_definition = SCM_CDR (canonical_definition);
     const SCM variable = SCM_CAR (cdr_canonical_definition);
+    const SCM value = scm_eval_car (SCM_CDR (cdr_canonical_definition), env);
     const SCM location
       = scm_sym2var (variable, scm_env_top_level (env), SCM_BOOL_T);
-    const SCM value = scm_eval_car (SCM_CDR (cdr_canonical_definition), env);
 
     if (SCM_REC_PROCNAMES_P)
       {


--- orig/test-suite/ChangeLog
+++ mod/test-suite/ChangeLog
@@ -1,3 +1,11 @@
+2007-05-26  Ludovic Courtès  <address@hidden>
+
+       * tests/syntax.test (top-level define)[binding is created before
+       expression is evaluated]: Moved to "internal define", using `let'
+       instead of `begin'.  The test was not necessarily valid for
+       top-level defines, according to Section 5.2.1 or R5RS.
+       [redefinition]: New.
+
 2007-05-09  Ludovic Courtès  <address@hidden>
 
        * tests/srfi-19.test ((current-time time-tai) works): Use `time?'.


--- orig/test-suite/tests/syntax.test
+++ mod/test-suite/tests/syntax.test
@@ -725,15 +725,16 @@
 
 (with-test-prefix "top-level define"
 
-  (pass-if "binding is created before expression is evaluated"
-    (= (eval '(begin
-                (define foo
-                  (begin
-                    (set! foo 1)
-                    (+ foo 1)))
-                foo)
-             (interaction-environment))
-       2))
+  (pass-if "redefinition"
+    (let ((m (make-module)))
+      (beautify-user-module! m)
+
+      ;; The previous value of `round' must still be visible at the time the
+      ;; new `round' is defined.  According to R5RS (Section 5.2.1), `define'
+      ;; should behave like `set!' in this case (except that in the case of
+      ;; Guile, we respect module boundaries).
+      (eval '(define round round) m)
+      (eq? (module-ref m 'round) round)))
 
   (with-test-prefix "currying"
 
@@ -780,6 +781,17 @@
                   (eq? 'c (a 2) (a 5))))
           (interaction-environment)))
 
+  (pass-if "binding is created before expression is evaluated"
+    ;; Internal defines are equivalent to `letrec' (R5RS, Section 5.2.2).
+    (= (eval '(let ()
+                (define foo
+                  (begin
+                    (set! foo 1)
+                    (+ foo 1)))
+                foo)
+             (interaction-environment))
+       2))
+
   (pass-if "internal defines with begin"
     (false-if-exception
      (eval '(let ((a identity) (b identity) (c identity))