bug#24923: 25.1; Lisp watchpoints

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Date: Thu, 10 Nov 2016 22:10:38 -0500
> 
> Continuing from
> https://lists.nongnu.org/archive/html/emacs-devel/2015-11/msg01437.html,
> main code difference since then is that I use a subr instead of indexing
> into a table of C functions (the reason for using an index was to avoid
> GC on Ffuncall; instead I did that by checking if the function is SUBRP
> and doing funcall_subr on it (a new function extracted from Ffuncall)).

Thanks.  A few comments below.

One general comment is that these patches are harder to review due to
whitespace changes TAB->spaces.  How about using some non-default
options to "git diff" when generating the patch?

> >From 0507854b885681c2e57bb1c6cb97f898417bd99c Mon Sep 17 00:00:00 2001

This changeset was attached twice, for some reason.

> --- a/src/data.c
> +++ b/src/data.c
> @@ -649,9 +649,10 @@ DEFUN ("makunbound", Fmakunbound, Smakunbound, 1, 1, 0,
>    (register Lisp_Object symbol)
>  {
>    CHECK_SYMBOL (symbol);
> -  if (SYMBOL_CONSTANT_P (symbol))
> +  if (XSYMBOL (symbol)->trapped_write == SYMBOL_NOWRITE)
>      xsignal1 (Qsetting_constant, symbol);
> -  Fset (symbol, Qunbound);
> +  else
> +    Fset (symbol, Qunbound);
>    return symbol;

Why was this needed?  Doesn't xsignal1 never return anymore?

> +static void
> +reset_symbol_trapped_write (Lisp_Object symbol)
> +{
> +  set_symbol_trapped_write (symbol, SYMBOL_TRAPPED_WRITE);
> +}

Suggest to find a better name for this function, like
restore_symbol_trapped_write, for example.  Calling an action that
sets an attribute "reset" makes it harder to read the code.

> +DEFUN ("add-variable-watcher", Fadd_variable_watcher, Sadd_variable_watcher,
> +       2, 2, 0,
> +       doc: /* Cause WATCH-FUNCTION to be called when SYMBOL is set.  */)

I think the doc string needs to mention that the function also affects
all of SYMBOL's aliases.

> +DEFUN ("remove-variable-watcher", Fremove_variable_watcher, 
> Sremove_variable_watcher,
> +       2, 2, 0,
> +       doc: /* Undo the effect of `add-variable-watcher'.  */)

The doc string should mention SYMBOL.  Also, preferably have the doc
string be self-contained, since that's easy in this case.

> +  (Lisp_Object symbol, Lisp_Object watch_function)
> +{
> +  symbol = Findirect_variable (symbol);
> +  Lisp_Object watchers = Fget (symbol, Qwatchers);
> +  watchers = Fdelete (watch_function, watchers);
> +  if (NILP (watchers))
> +    {
> +      set_symbol_trapped_write (symbol, SYMBOL_UNTRAPPED_WRITE);
> +      map_obarray (Vobarray, harmonize_variable_watchers, symbol);
> +    }
> +  Fput (symbol, Qwatchers, watchers);
> +  return Qnil;

Would it be more useful for this and add-variable-watcher to return
the list of watchers instead?

> +  /* Avoid recursion.  */
> +  set_symbol_trapped_write (symbol, SYMBOL_UNTRAPPED_WRITE);
> +  ptrdiff_t count = SPECPDL_INDEX ();
> +  record_unwind_protect (reset_symbol_trapped_write, symbol);

I think record_unwind_protect should be called before setting the
attribute, for this code to be safer and more future-proof.

> +/* Value is non-zero if symbol cannot be changed through a simple set,
> +   i.e. it's a constant (e.g. nil, t, :keywords), or it has some
> +   watching functions.  */
>  
>  INLINE int
> -(SYMBOL_CONSTANT_P) (Lisp_Object sym)
> +(SYMBOL_TRAPPED_WRITE_P) (Lisp_Object sym)
>  {
> -  return lisp_h_SYMBOL_CONSTANT_P (sym);
> +  return lisp_h_SYMBOL_TRAPPED_WRITE_P (sym);
>  }
>  
>  /* Placeholder for make-docfile to process.  The actual symbol
> @@ -3289,6 +3299,12 @@ set_symbol_next (Lisp_Object sym, struct Lisp_Symbol 
> *next)
>    XSYMBOL (sym)->next = next;
>  }
>  
> +INLINE void
> +make_symbol_constant (Lisp_Object sym)
> +{
> +  XSYMBOL (sym)->trapped_write = SYMBOL_NOWRITE;
> +}
> +

So we will have make_symbol_constant, but no SYMBOL_CONSTANT_P?  Isn't
that confusing?  Some C code may wish to know whether a symbol is
constant, not just either constant or trap-on-write; why not give them
what they want?

> +      ;; Watchpoint triggered.
> +      ((and `watchpoint (let `(,symbol ,newval . ,details) (cdr args)))
> +       (insert
> +        "--"
> +        (pcase details
> +          (`(makunbound ,_) (format "Making %s void" symbol))
> +          (`(let ,_) (format "let-binding %s to %S" symbol newval))
> +          (`(unlet ,_) (format "ending let-binding of %s" symbol))
> +          (`(set nil) (format "setting %s to %S" symbol newval))
> +          (`(set ,buffer) (format "setting %s in %s to %S"
                                     ^^^^^^^^^^^^^^^^^^^^^^^^
IMO, this should include the word "buffer", otherwise it can be
confusing.

> +          (_ (format "watchpoint triggered %S" (cdr args))))

Can you give a couple of examples of this, with %S shown explicitly?
I'm not sure whether the result will be self-explanatory.

> +;;;###autoload
> +(defun debug-watch (variable)
> +  (interactive
> +   (let* ((var-at-point (variable-at-point))
> +          (var (and (symbolp var-at-point) var-at-point))
> +          (val (completing-read
> +                (concat "Debug when setting variable"
> +                        (if var (format " (default %s): " var) ": "))

I think all the other commands/variables similar to this one are named
debug-on-SOMETHING, so perhaps this one should also have such a name.
Like debug-on-setting-variable, perhaps?

> +** New function `add-variable-watcher' can be used to execute code
                                                      ^^^^^^^^^^^^^^^
It is better to say "to call a function" here.

Thanks again for working on this.