Re: [PATCH] scm_module_variable, duplicate bindings handlers

[Ludovic Courtès]

Hi Andy,

Andy Wingo <address@hidden> 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.