Re: [PATCH] (Updated) Run hook when variable is set

[Stefan Monnier <monnier@IRO.UMontreal.CA>]

>> No, the idea was rather to do:
>> 
>> (defun set-internal-1 (args)
>> (block nil ; Because Elisp isn't CL
>> (if (constant-or-hooked-p)
>> (if (constant-p)
>> (if (forbidden-p)
>> (error "setting constant")
>> (return))
>> (funcall symbol-watch-function ..args..))
>> (do-some-stuff)
>> (and-many-more-lines-of-stuff)
>> (set-some-variable))))

> Then the many more lines of stuff have to be copied into
> symbol-watch-function. Those lines can't just be skipped;

We don't really need to copy those lines, instead symbol-watch-function
should somehow (in the "normal" case) call us back, and in way that
doesn't trigger the (constant-or-hooked-p) check.

Of course, for that "...args..." should have enough info that we can
indeed "call it back" properly.

> By default, hooking a symbol must not change the behavior of Emacs,
> except for slowing it down.  A hook handler can optionally change the
> behavior, but just hooking the symbol must cause no change if the
> handler doesn't.

Agreed.

> Updated patches attached. The first applies to 24.4.

We don't want to install this in emacs-24, so only the trunk (aka
"master") code is important in this respect.
[ BTW, even for 30KB patches, I prefer them uncompressed, because
  I typically read them directly in Gnus.  ]

> +  /* When masked with SYMBOL_CONSTANT_MASK, non-zero means symbol is
> +     constant, i.e. changing its value should signal an error.  If the
> +     value is 3, then the var can be changed, but only by `defconst'.
> +     When masked with SYMBOL_HOOKED_MASK, non-zero means setting
> +     symbol will run varhook.  These two fields are combined into one
> +     in order to optimize the fast path of unhooked non-constants by
> +     having only one conditional branch for that case.  */

We don't need this special value 3 for defconst, not only because the
code doesn't use it, but also since if we want that unimplemented
functionality, we can now implement it using your new
symbol-setter system.

> +typedef enum
> +  {
> +    Dyn_Unbind = -1,
> +    Dyn_Current = 0,
> +    Dyn_Bind = 1,
> +    Dyn_Skip = 2,
> +    Dyn_Global = 3
> +  } Dyn_Bind_Direction;

In which sense is this a "direction"?

> +/* Like set_internal but with direction argument to indicate whether this
> +   function call is due to a binding (1), an unbinding (-1), or neither (0).
> +   As special cases, a value of 2 is a flag to disable run_varhook so that
> +   varhooks aren't run during backtraces, and a value of 3 is a flag
> +   indicating that this function call is due to set_default, which allows
> +   run_varhook to distinguish beween the global and the dyn-local binding.  */

Please use the Dyn_* names rather than the numerical constants in
the comment.

> +   Return the result of symbol-setter-function. The variable will be set
> +   (by code that calls run_varhook) to that result, overriding the value to
> +   which the setter is attempting to set the variable.  */

That's a good idea, to circumvent the question of how to not-trigger the
hooked-p check recursively when the hook function calls the setter (tho
the question partly remains, in case the hook function *accidentally*
sets one of the hooked variables).

It does mean that the hooks can't redirect the assignment elsewhere, but
maybe it's a good thing anyway.

> +	bool shadowed;
> +	if (buf_local)
> +	  shadowed = let_shadows_buffer_binding_p (sym);
> +	else shadowed = let_shadows_global_binding_p (symbol);

Aka
        bool shadowed = (buf_local ? let_shadows_buffer_binding_p (sym)
                         : let_shadows_global_binding_p (symbol));

> +	if (shadowed) env = Qdyn_local;
> +	else if (buf_local) env = Qbuf_local;
> +	else env = Qglobal;

Why does the hook need to know about those different cases?

> + start: /* Just to avoid reindentation in the varhook patch */

"diff -bw" works just as well for me.

> +			 (XFWDTYPE (innercontents))==Lisp_Fwd_Buffer_Obj,
                                                   ^^
We like to put spaces around infix operators.

> +DEFUN ("symbol-setter-function", Fsymbol_setter_function, Ssymbol_setter_function, 4, 4, 0,

Hmm, no symbol-setter-function should be a variable (holding
a function), modified via add-function/remove-function.

Also the docstring should not recommend :override (which should be
a rather rare case, the more useful cases are probably :before
and :around), and in general it should just say how the function is
called and what it should return, without giving a primer about how to
modify such *-function variables, since we don't want to do that for
every *-function variable.

OTOH the docstring should document the different possible values of ENV
and their respective meaning.


        Stefan