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

[Stefan Monnier]

> I'm unclear on your reason for extending the «constant» field to
> include the «hooked» bit, rather than giving the latter its own name.

I thought it was clear: so that the new feature does not cost anything
as long as you don't use it.  I.e. that extends existing tests of
"constant is 0" to mean both "not constant" and "not hooked either".

> Either way, a new bit is needed (I can't fit the meaning of «hooked»
> into «constant»'s current two bits)

Why not?  Currently `constant' occupies two bits but is used as
a boolean value.  So there's a bit available right there.  And if not,
you can add a bit to that field.

> Example usage:

I don't understand your example: what's `varhook'?  What's `hook'?
What's `unhook'?

> +INLINE void
> +try_run_varhook (struct Lisp_Symbol* sym, bool buf_local,
> +              Dyn_Bind_Direction dir, Lisp_Object value)
> +{
> +  if (sym->hooked)
> +    run_varhook (sym, buf_local, dir, value);
> +}

The hook should be called *to perform the assignment*, rather than
calling it after performing the assignment ourselves.
And it should not use run_hook* but funcall (i.e. the hook should be
manipulated in Elisp via `add-function' rather than via `add-hook').

> -extern Lisp_Object run_hook_with_args (ptrdiff_t nargs, Lisp_Object *args,
> -                                    Lisp_Object (*funcall)
> -                                    (ptrdiff_t nargs, Lisp_Object *args));
> +extern Lisp_Object run_hook_with_args (ptrdiff_t, Lisp_Object *,
> +                                    Lisp_Object (*) (ptrdiff_t, Lisp_Object 
> *));

What for?

>       if (!sym->constant)
> -     SET_SYMBOL_VAL (sym, value);
> +     {
> +       SET_SYMBOL_VAL (sym, value);
> +       try_run_varhook (sym, false, Dyn_Bind, value);
> +     }

You just slowed down all the code even if the feature is not used.
I think we can do better.

>           if (sym->redirect == SYMBOL_PLAINVAL)
>             {
> -             SET_SYMBOL_VAL (sym, specpdl_old_value (specpdl_ptr));
> +             Lisp_Object oldval = specpdl_old_value (specpdl_ptr);
> +             SET_SYMBOL_VAL (sym, oldval);
> +             try_run_varhook (sym, false, Dyn_Unbind, oldval);

Same here.

> +run_varhook (struct Lisp_Symbol* sym, bool buf_local, Dyn_Bind_Direction dir,
> +          Lisp_Object value)
> +{
> +  Lisp_Object hook_and_args[4];
> +  if (dir == Dyn_Skip) /* From backtrace_eval_unrewind */
> +    return;

Hmm... didn't think of backtrace_eval_unrewind.  Indeed, I think we
don't want this to trigger a hook (well, maybe there are cases where
we'd want this, but I don't think the current code can handle it
correctly/safely).

> +  hook_and_args[0] = Qvarhook;
> +  XSETSYMBOL (hook_and_args[1], sym);
> +  switch (dir)
> +    {
> +    case Dyn_Current:
> +      {
> +     bool shadowed;
> +     if (buf_local)
> +       shadowed = let_shadows_buffer_binding_p (sym);
> +     else shadowed = let_shadows_global_binding_p (hook_and_args[1]);
> +     if (shadowed) hook_and_args[2] = Qdyn_local;
> +     else if (buf_local) hook_and_args[2] = Qbuf_local;
> +     else hook_and_args[2] = Qglobal;
> +     break;
> +      }
> +    case Dyn_Global:
> +      {
> +     if (let_shadows_global_binding_p (hook_and_args[1]))
> +       hook_and_args[2] = Qinvalid;
> +     else hook_and_args[2] = Qglobal;
> +     break;
> +      }
> +    case Dyn_Bind: hook_and_args[2] = Qdyn_bind; break;
> +    case Dyn_Unbind: hook_and_args[2] = Qdyn_unbind; break;
> +    default: emacs_abort ();
> +    }
> +  hook_and_args[3] = value;
> +  run_hook_with_args (4, hook_and_args, funcall_nil);

I think I'd rather turn `dir' into a Lisp_Object (a symbol) and send
this directly to the hook.  And then, if/when needed we can try and
make available to Elisp the functionality of things like
let_shadows_*_binding_p.

BTW in order to keep the performance cost of this patch to a minimum, we
may end up reducing its precision.

> +  DEFSYM (Qvarhook, "varhook");

Ah, here it is.  Please use a name more along the lines of Elisp style.
E.g. `variable-watch-function'.

> +  defsubr (&Shook);
> +  defsubr (&Sunhook);
> +  defsubr (&Shookedp);

Use less generic names.  E.g. `variable-watch-set',
`variable-watch-unset', and `variable-watch-p'.


        Stefan