[PATCH] scm_module_variable, duplicate bindings handlers

[PATCH] scm_module_variable, duplicate bindings handlers

[Andy Wingo]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

Hey folks,

I've gotten guile-gnome to run against guile HEAD now, with the help of
the following patches:

 1) misordered-module-variable-lookup.patch, fixes the lookup order in
 scm_module_variable to be like 1.8, which allows binders to run at the
 correct time

 2) duplicate-binding-use-module-variable.patch, fixes the duplicate
 bindings handlers to lookup the resolved values using module-variable
 rather than module-local-variable, as it is possible that public
 interfaces use other modules.

Cheers,

Andy.
-- 
http://wingolog.org/


[-- Attachment #2: misordered-module-variable-lookup.patch --]
[-- Type: text/x-diff, Size: 1972 bytes --]

--- ChangeLog.~1.2396.~	2007-08-04 16:11:09.000000000 +0200
+++ ChangeLog	2007-08-10 11:41:05.000000000 +0200
@@ -1,3 +1,9 @@
+2007-08-10  Andy Wingo  <wingo@pobox.com>
+
+	* modules.c (scm_module_variable, scm_module_local_variable): Fix
+	misordered lookup sequence: it should be obarray, binder, imports
+	rather than obarray, imports, binder.
+
 2007-07-29  Ludovic Courtès  <ludo@gnu.org>
 
 	* Makefile.am (INCLUDES): Added Gnulib includes.
--- modules.c.~1.65.~	2007-05-05 22:38:57.000000000 +0200
+++ modules.c	2007-08-10 11:33:50.000000000 +0200
@@ -418,15 +418,8 @@
   if (SCM_BOUND_THING_P (b))
     return b;
 
-  /* 2. Search imported bindings.  In order to be consistent with
-     `module-variable', the binder gets called only when no imported binding
-     matches SYM.  */
-  b = module_imported_variable (module, sym);
-  if (SCM_BOUND_THING_P (b))
-    return SCM_BOOL_F;
-
   {
-    /* 3. Query the custom binder.  */
+    /* 2. Query the custom binder.  */
     SCM binder = SCM_MODULE_BINDER (module);
 
     if (scm_is_true (binder))
@@ -437,8 +430,11 @@
       }
   }
 
-  return SCM_BOOL_F;
 
+  /* 3. No need to search imported bindings, because we are only looking for
+     local variables.  */
+
+  return SCM_BOOL_F;
 #undef SCM_BOUND_THING_P
 }
 #undef FUNC_NAME
@@ -465,13 +461,8 @@
   if (SCM_BOUND_THING_P (var))
     return var;
 
-  /* 2. Search among the imported variables.  */
-  var = module_imported_variable (module, sym);
-  if (SCM_BOUND_THING_P (var))
-    return var;
-
   {
-    /* 3. Query the custom binder.  */
+    /* 2. Query the custom binder.  */
     SCM binder;
 
     binder = SCM_MODULE_BINDER (module);
@@ -483,6 +474,11 @@
       }
   }
 
+  /* 3. Search among the imported variables.  */
+  var = module_imported_variable (module, sym);
+  if (SCM_BOUND_THING_P (var))
+    return var;
+
   return SCM_BOOL_F;
 
 #undef SCM_BOUND_THING_P

[-- Attachment #3: duplicate-binding-use-module-variable.patch --]
[-- Type: text/x-diff, Size: 1190 bytes --]

--- ChangeLog.~1.682.~	2007-05-05 22:38:56.000000000 +0200
+++ ChangeLog	2007-08-10 11:43:49.000000000 +0200
@@ -1,3 +1,10 @@
+2007-08-10  Andy Wingo  <wingo@pobox.com>
+
+	* boot-9.scm (duplicate-handlers): When looking for a variable's
+	binding in a interface, use module-variable rather than
+	module-local-variable, as public interfaces can also use other
+	modules.
+
 2007-05-05  Ludovic Courtès  <ludo@chbouib.org>
 
 	Implemented lazy duplicate binding handling.  Fixed the
--- boot-9.scm.~1.361.~	2007-05-05 22:38:56.000000000 +0200
+++ boot-9.scm	2007-08-09 13:42:44.000000000 +0200
@@ -3088,13 +3088,13 @@
 		     (module-name module)
 		     (module-name int2)
 		     name)
-	     (module-local-variable int2 name))))
+	     (module-variable int2 name))))
      
     (define (first module name int1 val1 int2 val2 var val)
-      (or var (module-local-variable int1 name)))
+      (or var (module-variable int1 name)))
      
     (define (last module name int1 val1 int2 val2 var val)
-      (module-local-variable int2 name))
+      (module-variable int2 name))
      
     (define (noop module name int1 val1 int2 val2 var val)
       #f)

[-- Attachment #4: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: [PATCH] scm_module_variable, duplicate bindings handlers

[Ludovic Courtès]

Hi Andy,

Andy Wingo <wingo@pobox.com> writes:

>  1) misordered-module-variable-lookup.patch, fixes the lookup order in
>  scm_module_variable to be like 1.8, which allows binders to run at the
>  correct time

See comments below.

>  2) duplicate-binding-use-module-variable.patch, fixes the duplicate
>  bindings handlers to lookup the resolved values using module-variable
>  rather than module-local-variable, as it is possible that public
>  interfaces use other modules.

Ok, sounds good.

> --- modules.c.~1.65.~	2007-05-05 22:38:57.000000000 +0200
> +++ modules.c	2007-08-10 11:33:50.000000000 +0200
> @@ -418,15 +418,8 @@
>    if (SCM_BOUND_THING_P (b))
>      return b;
>  
> -  /* 2. Search imported bindings.  In order to be consistent with
> -     `module-variable', the binder gets called only when no imported binding
> -     matches SYM.  */
> -  b = module_imported_variable (module, sym);
> -  if (SCM_BOUND_THING_P (b))
> -    return SCM_BOOL_F;
> -
>    {
> -    /* 3. Query the custom binder.  */
> +    /* 2. Query the custom binder.  */

The rationale here was to be consistent with `module-variable', but it
admittedly sounds questionable to look for imported variables here.

> -  /* 2. Search among the imported variables.  */
> -  var = module_imported_variable (module, sym);
> -  if (SCM_BOUND_THING_P (var))
> -    return var;
> -
>    {
> -    /* 3. Query the custom binder.  */
> +    /* 2. Query the custom binder.  */

Here, the rationale was that invoking the custom binder is expensive and
should be done as a last resort.

OTOH, given how `module-autoload!' is implemented, it probably doesn't
make any difference performance-wise to do things in the order you
suggest, and it seems more consistent since it allows local variables to
always override imported variables.

So both patches make sense.

You'll need to assign copyright to the FSF for Guile, though.  We can
discuss it privately if you want.

Thanks!

Ludovic.



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: [PATCH] scm_module_variable, duplicate bindings handlers

[Neil Jerram]

ludo@gnu.org (Ludovic Courtès) writes:

> So both patches make sense.

Agreed, but you may also want to write some tests, to check and
document the behaviour that you expect for guile-gnome.  Otherwise
there's nothing to catch someone accidentally reverting this change
again.

        Neil



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel