bug#3467: 23.0.94; let + make-local-variable => let value made global

[Lennart Borgman <lennart.borgman@gmail.com>]

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

On Sat, Jun 6, 2009 at 12:48 AM, Lennart
Borgman<lennart.borgman@gmail.com> wrote:
>
> Sigh, and my suggestion was of course unnecessary stupid. What is
> needed is of course to record values and frame+buffer localness and
> dito values and reset them. Nothing less than this will ever work
> correctly, or?
>
> And does not this apply to all uses of specbind + unbind_to?
>
> Can it be sufficient to just change specbind and unbind_to? Is there
> anything else that will be affected by changes in the specbind stack?
> Since info about buffer+frame is alwas needed should specbinding be
> changed to the below form?
>
> struct specbinding
>  {
>    Lisp_Object symbol;
>    Lisp_Object old_value;
>    Lisp_Object old_buffer_value;
>    Lisp_Object old_frame_value;
>    specbinding_func func;
>    Lisp_Object unused;         /* Dividing by 16 is faster than by 12 */
>    Lisp_Object unused;
>    Lisp_Object unused;
>  };

Here is a bit modified suggestion and some code where I try to
implement it. Please notice that I have not tested the code. It is
very possible that I have misunderstood something so please look at
the code.

struct specbinding
  {
    Lisp_Object symbol;
    Lisp_Object old_value;
    specbinding_func func;
    // total 8 fields
    Lisp_Object need_test_local;
    Lisp_Object old_buffer;
    Lisp_Object old_buffer_value;
    Lisp_Object old_frame;
    Lisp_Object old_frame_value;
    //Lisp_Object unused;		/* Dividing by 16 is faster than by 12 */
  };

[-- Attachment #2: eval.diff --]
[-- Type: application/octet-stream, Size: 7463 bytes --]

*** eval.c	2009-06-06 22:06:33.359375000 +0200
--- eval-orig.c	2009-03-27 09:21:26.453125000 +0100
***************
*** 3298,3304 ****
        specpdl_ptr->symbol = symbol;
        specpdl_ptr->old_value = valcontents;
        specpdl_ptr->func = NULL;
-       //specpdl_ptr->need_test_local = Qnil; // no need to set this
        ++specpdl_ptr;
        SET_SYMBOL_VALUE (symbol, value);
      }
--- 3298,3303 ----
***************
*** 3306,3317 ****
      {
        Lisp_Object ovalue = find_symbol_value (symbol);
        specpdl_ptr->func = 0;
!       //specpdl_ptr->old_value = ovalue;
!       /* Fix-me: Store symbol global value in old_value */
!       specpdl_ptr->old_value = do_symval_forwarding (valcontents);
!       specpdl_ptr->need_test_local = Qt;
!       specpdl_ptr->old_buffer= Qunbound;
!       specpdl_ptr->old_frame= Qunbound;
  
        valcontents = XSYMBOL (symbol)->value;
  
--- 3305,3311 ----
      {
        Lisp_Object ovalue = find_symbol_value (symbol);
        specpdl_ptr->func = 0;
!       specpdl_ptr->old_value = ovalue;
  
        valcontents = XSYMBOL (symbol)->value;
  
***************
*** 3324,3349 ****
  
  	  /* For a local variable, record both the symbol and which
  	     buffer's or frame's value we are saving.  */
!           /* Fix-me: This only saves the current used local binding,
!              not both as it should */
! 	  if (!NILP (Flocal_variable_p (symbol, Qnil))) {
  	    where = current_buffer;
!             specpdl_ptr->old_buffer = where;
!             specpdl_ptr->old_buffer_value = ovalue;
!           } else if (BUFFER_LOCAL_VALUEP (valcontents)
!                      && XBUFFER_LOCAL_VALUE (valcontents)->found_for_frame) {
  	    where = XBUFFER_LOCAL_VALUE (valcontents)->frame;
!             specpdl_ptr->old_frame = where;
!             specpdl_ptr->old_frame_value = ovalue;
! 	  } else {
  	    where = Qnil;
-           }
  
  	  /* We're not using the `unused' slot in the specbinding
  	     structure because this would mean we have to do more
  	     work for simple variables.  */
! 	  //specpdl_ptr->symbol = Fcons (symbol, Fcons (where, current_buffer));
!           specpdl_ptr->symbol = symbol;
  
  	  /* If SYMBOL is a per-buffer variable which doesn't have a
  	     buffer-local value here, make the `let' change the global
--- 3318,3335 ----
  
  	  /* For a local variable, record both the symbol and which
  	     buffer's or frame's value we are saving.  */
! 	  if (!NILP (Flocal_variable_p (symbol, Qnil)))
  	    where = current_buffer;
! 	  else if (BUFFER_LOCAL_VALUEP (valcontents)
! 		   && XBUFFER_LOCAL_VALUE (valcontents)->found_for_frame)
  	    where = XBUFFER_LOCAL_VALUE (valcontents)->frame;
! 	  else
  	    where = Qnil;
  
  	  /* We're not using the `unused' slot in the specbinding
  	     structure because this would mean we have to do more
  	     work for simple variables.  */
! 	  specpdl_ptr->symbol = Fcons (symbol, Fcons (where, current_buffer));
  
  	  /* If SYMBOL is a per-buffer variable which doesn't have a
  	     buffer-local value here, make the `let' change the global
***************
*** 3419,3479 ****
  	 binding.  WHERE nil means that the variable had the default
  	 value when it was bound.  CURRENT-BUFFER is the buffer that
           was current when the variable was bound.  */
!       /* else if (CONSP (this_binding.symbol)) */
!       /*   { */
!       /*     Lisp_Object symbol, where; */
! 
!       /*     symbol = XCAR (this_binding.symbol); */
!       /*     where = XCAR (XCDR (this_binding.symbol)); */
! 
!       /*     if (NILP (where)) */
!       /*       Fset_default (symbol, this_binding.old_value); */
!       /*     else if (BUFFERP (where)) */
!       /*       set_internal (symbol, this_binding.old_value, XBUFFER (where), 1); */
!       /*     else */
!       /*       set_internal (symbol, this_binding.old_value, NULL, 1); */
!       /*   } */
!       /* else */
!       /*   { */
!       /*     /\* If variable has a trivial value (no forwarding), we can */
!       /*        just set it.  No need to check for constant symbols here, */
!       /*        since that was already done by specbind.  *\/ */
!       /*     if (!MISCP (SYMBOL_VALUE (this_binding.symbol))) */
!       /*       SET_SYMBOL_VALUE (this_binding.symbol, this_binding.old_value); */
!       /*     else */
!       /*       set_internal (this_binding.symbol, this_binding.old_value, 0, 1); */
!       /*   } */
!       else {
!         /* Fix-me: Before restoring anything the buffer/frame
!            localness must be checked.  I am not sure how to do that,
!            but see below for my attempts.
!         */
!         /* Set global value */
          if (!MISCP (SYMBOL_VALUE (this_binding.symbol)))
            SET_SYMBOL_VALUE (this_binding.symbol, this_binding.old_value);
          else
!           set_default (this_binding.symbol, this_binding.old_value, 0, 1);
! 
!         {
!           register Lisp_Object valcontents = SYMBOL_VALUE (symbol);
!           int is_local_now = BUFFER_LOCAL_VALUEP (valcontents) || BUFFER_OBJFWDP (valcontents);
!           int is_buffer_local_now = is_local_now && Flocal_variable_p (this_binding.symbol, Qnil);
!           int is_frame_local_now = is_local_now && !is_buffer_local_now;
!           int was_buffer_local_before = this_binding.old_buffer /= Qunbound;
!           int was_local_before = (was_buffer_local_before ||
!                                   this_binding.old_frame /= Qunbound);
!           int was_frame_local_before = (was_local_before && ! was_buffer_local_before);
!           /* Fix me: restore localness */
!           if (was_buffer_local_before_specbind) {
!             if (!is_buffer_local_now) Fmake_local_variable (this_binding.symbol);
!             /* Set buffer local value */
!             set_internal (symbol, this_binding.old_buffer_value, XBUFFER (this_binding.old_buffer), 1);
!           } else if (was_frame_local_before_specbind) {
!             if (!is_frame_local_now) Fmake_variable_frame_local (this_binding.symbol);
!             /* Set frame local value */
!             set_internal (symbol, this_binding.old_frame_value, NULL, 1);
!           }
!         }
        }
      }
  
--- 3405,3433 ----
  	 binding.  WHERE nil means that the variable had the default
  	 value when it was bound.  CURRENT-BUFFER is the buffer that
           was current when the variable was bound.  */
!       else if (CONSP (this_binding.symbol))
! 	{
! 	  Lisp_Object symbol, where;
! 
! 	  symbol = XCAR (this_binding.symbol);
! 	  where = XCAR (XCDR (this_binding.symbol));
! 
! 	  if (NILP (where))
! 	    Fset_default (symbol, this_binding.old_value);
! 	  else if (BUFFERP (where))
! 	    set_internal (symbol, this_binding.old_value, XBUFFER (where), 1);
! 	  else
! 	    set_internal (symbol, this_binding.old_value, NULL, 1);
! 	}
!       else
! 	{
! 	  /* If variable has a trivial value (no forwarding), we can
! 	     just set it.  No need to check for constant symbols here,
! 	     since that was already done by specbind.  */
  	  if (!MISCP (SYMBOL_VALUE (this_binding.symbol)))
  	    SET_SYMBOL_VALUE (this_binding.symbol, this_binding.old_value);
  	  else
! 	    set_internal (this_binding.symbol, this_binding.old_value, 0, 1);
  	}
      }