Re: Lisp watchpoints

[Stefan Monnier]

> @@ -629,9 +634,18 @@ DEFUN ("makunbound", Fmakunbound, Smakunbound, 1, 1, 0,
>    (register Lisp_Object symbol)
>  {
>    CHECK_SYMBOL (symbol);
> -  if (SYMBOL_CONSTANT_P (symbol))
> -    xsignal1 (Qsetting_constant, symbol);
> -  Fset (symbol, Qunbound);
> +  switch (XSYMBOL (symbol)->trapped_write)
> +    {
> +    case SYMBOL_NOWRITE:
> +      xsignal1 (Qsetting_constant, symbol);
> +
> +    case SYMBOL_TRAPPED_WRITE:
> +      notify_variable_watchers (symbol, Qnil, Qunbind, Qnil);
> +      /* fallthrough */
> +    case SYMBOL_UNTRAPPED_WRITE:
> +    default:
> +      Fset (symbol, Qunbound);
> +    }
>    return symbol;
>  }

Hmm... `unbind' is the term usually used when exiting a let binding
rather than when setting a var to Qunbound.  So I think it'd be better
here to use Qmakunbound to avoid confusion.

> +  set_symbol_trapped_write (symbol, SYMBOL_UNTRAPPED_WRITE); /* avoid 
> recursion */

Please capitalize and punctaute your comments.  Also, try not to exceed
80 columns.

> +  ptrdiff_t count = SPECPDL_INDEX ();
> +  record_unwind_protect (&reset_symbol_trapped_write, symbol);
                           ^^^
I think this "&" is unneeded.

> +  while (!NILP (watchers))
> +    {
> +      Lisp_Object watcher = XCAR (watchers);

This will lead to a segmentation fault after something like (put <var>
'watchers [1]).  While very unlikely, it's very easy to avoid this
problem: just use CONSP instead of !NILP.

> +      else if (FUNCTIONP (watcher))
> +        CALLN (Ffuncall, watcher, operation, where, symbol, newval);

I don't think you need to test "FUNCTIONP (watcher)".
And you don't need Ffuncall: what you wrote is similar to writing

   (funcall #'funcall watcher operation where symbol newval)

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

Actually, trapped writes won't work reliably for frame-local vars
(because you can set them with things like `put' or `setcar' rather than
let/setq/...), so better disallow combining the two (frame-local vars
are obsolete anyway and should be removed "soonish", so it's fine not
to provide support for new features for them).


        Stefan