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

[Stefan Monnier <monnier@iro.umontreal.ca>]

> blocking/overriding the variable setting: if you hook a variable and
> override its setting, then the return value of set, setq, etc should be the
> override value instead of the originally attempted value, so that the
> override will cascade through e.g. (setq x (setq y foo)) and (if (setq
> z foo) ...).

I think either behavior is equally correct and wrong (sometimes you'd
rather want one and sometimes you'd rather want the other), so I'd
rather choose based on which of the two is more efficient/easy to
implement (and if it's not on the critical path (i.e. the path used when
the variable is not hooked), then efficiency doesn't matter).

In either case, `setq' should never return Qunbound.

> Any other changes I should make?

Well, now that you ask:

Regarding one of the bikesheds, I think "constant" is a better name than
"vetted" because the name really doesn't matter that much, "vetted" is
not very self-explanatory, and "constant" has the major advantage of
making the patch more readable by eliminating "irrelevant" changes.
We can change the name later to something actually better, but for now,
let's just stick to "constant".

> +/* These are the masks for the vetted field of Lisp_Symbol.
> +   Bit 0 stores the constant field.  Bit 1 stores the hooked field.  */
> +#define SYM_CONST 1
> +#define SYM_HOOKED 2

constant and hooked at the same time is nonsensical, so these aren't two
independent bits, instead `vetted' is a 3-valued field.  An `enum'
sounds right.

> -static void grow_specpdl (void);
> +static inline void grow_specpdl (void);

What happens if we don't inline grow_specpdl?

> +      if (!sym->vetted) SET_SYMBOL_VAL (sym, value);
> +      else if (SYM_HOOKED_P (sym))

Please don't put the "then" branch on the same line as the test, this is
contrary to our coding conventions.

> +    case SYMBOL_VARALIAS:
> +      sym = indirect_variable (sym); XSETSYMBOL (symbol, sym); goto start;

Why did you have to move this?

> +	      set_internal (symbol, old_value, where, true, Dyn_Unbind);

If using "unbind" when referring to "undoing a let-binding" bothers you,
you could use "Dyn_Unwind" to mean that it's set as part of unwinding
the stack of bindings.  Personally I have no problem with using "unbind"
sometimes for "makunbound" and sometimes for "undo a let".

> +  DEFSYM (Qvoid_sentinel, "void-sentinel");
> +  DEFVAR_LISP ("void-sentinel", Vvoid_sentinel,
> +	       doc: /* Representation of voidness for hooked variables.
> +The value of this constant is an uninterned Lisp symbol that represents void
> +when passed to or returned from `symbol-setter-function'.  */);
> +  Vvoid_sentinel = Fmake_symbol (build_string ("::void::"));

I'd give it a more verbose name, to make sure it doesn't clash with
someone else's var.  After all, it's not like this is going to be used
very often, so the length doesn't matter.  `symbol-setter-void-value'
sounds fine.

> +  XSYMBOL (Vvoid_sentinel)->declared_special = true;
> +  XSYMBOL (Vvoid_sentinel)->vetted = SYM_CONST;

This value is just a symbol, not a variable, so there's no point marking
it as special and/or constant.

> +buf-local: The setter's buffer-local environment.
> +dyn-local: The innermost dynamic environment in which SYMBOL is bound.
> +dyn-bind: A new dynamic environment, such as creatable using `let'.

I think this needs clarification, because when I read it, I don't know
for sure which one I'd get in which case.

I think the info I'd want to get is:
- does it affect only the current buffer, or does it affect the default-value?
- is it a "set", or a "bind" or an "unbind"?
Note that those 2 aspects are orthogonal, so maybe we'd want 2 args
rather than 1.

> +If you use overriding advice, your advice must return the value to which to
> +set the variable.

Don't talk about advice here, just talk about what the function needs
to do.  After all, it may be modified without using advice.

> +If OLDVAL
> +is the value of void-sentinel but NEWVAL is not, you can override the new
> +value, but you can't prevent the variable from being set to a non-void value.

Why not?

> +To hook all variables of a symbol, use `symbol-hook'.

"all variables of a symbol" sounds really odd.  Whether there are
several variables for a single symbol is really a philosophical
question, and usually people seem to prefer thinking that it's not
the case.

>+  DEFVAR_LISP ("symbol-setter-function", Vsymbol_setter_function,
> +DEFUN ("symbol-hooked-p", Fsymbol_hooked_p, Ssymbol_hooked_p, 1, 1, 0,
> +DEFUN ("symbol-hook", Fsymbol_hook, Ssymbol_hook, 1, 1, 0,
> +DEFUN ("symbol-unhook", Fsymbol_unhook, Ssymbol_unhook, 1, 1, 0,

I'd much prefer to keep all the new functions/vars of this feature under
a common (and new) prefix.  I'll let you choose whether it should be
`symbol-setter-', `symbol-watcher-`, `symbol-hook-', or whatnot.

> -    case SYMBOL_VARALIAS: sym = indirect_variable (sym); goto start;
>      case SYMBOL_PLAINVAL: return SYMBOL_VAL (sym);
> +    case SYMBOL_VARALIAS: sym = indirect_variable (sym); goto start;

Why was this change needed?

> +  if (rawenv == Dyn_Makvoid && EQ (newval, Vvoid_sentinel))
> +    return Qunbound; /* Converting setq, etc to makunbound is prohibited. */
> +  return newval; /* So void_sentinel is ignored except for makunbound. */

Why disallow Qunbound for all but makunbound?
I understand that it might be strange to return Qunbound in the `setq'
case, but I'm not sure it's worth the trouble trying to disallow it.

And it seems positively wrong in the case of unwinding the topmost let
binding of a variable, so even if we do want to disallow it, the test
shouldn't be "rawenv == Dyn_Makvoid" but "orig_newval == Qunbound".
So, I think we don't need Dyn_Makvoid (hence we don't need to touch
Fmakunbound).

> -  set_internal (symbol, newval, Qnil, 0);
> -  return newval;
> +  return set_internal (symbol, newval, Qnil, false, Dyn_Current);

Since writing the above, I think I'm beginning to like the idea of
(setq <var> <val>) always returning <val> even if <var> ends up set to
something else.  After all, that's what we've been doing so far.

Try M-: (list (setq auto-hscroll-mode 2) auto-hscroll-mode) RET
 
> -    case SYMBOL_VARALIAS: sym = indirect_variable (sym); goto start;
>      case SYMBOL_PLAINVAL: return SYMBOL_VAL (sym);
> +    case SYMBOL_VARALIAS: sym = indirect_variable (sym); goto start;

Hmm... same kind of change here.  I must be missing something.

> -  if (sym->constant)
> +  if (SYM_CONST_P (sym))
>      error ("Symbol %s may not be frame-local", SDATA (SYMBOL_NAME (variable)));

I think we can keep sym->constant here.  If things don't work 100% for
frame-local vars, it's a good thing because we want to inflict pain on
the few rare remaining users of frame-local.


        Stefan