Re: Lisp watchpoints

[Eli Zaretskii]

> Date: Sat, 28 Nov 2015 21:04:25 -0500
> From: Noam Postavsky <address@hidden>
> Cc: Stefan Monnier <address@hidden>, John Wiegley <address@hidden>, 
> address@hidden
> 
> From c1e2e14097f4488384ea8ea3cab3cd51c41947eb Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <address@hidden>
> Date: Thu, 19 Nov 2015 19:50:06 -0500
> Subject: [PATCH v2 1/3] Add lisp watchpoints

Thanks.

> +DEFUN ("remove-variable-watcher", Fremove_variable_watcher, 
> Sremove_variable_watcher,
> +       2, 2, 0,
> +       doc: /* Cause WATCH-FUNCTION to be called when SYMBOL is set. */)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy/paste mistake?

> +static void
> +notify_variable_watchers (Lisp_Object symbol,
> +                          Lisp_Object newval,
> +                          Lisp_Object operation,
> +                          Lisp_Object where)
> +{
> +  Lisp_Object watchers = Fget (symbol, Qwatchers);
> +
> +  set_symbol_trapped_write (symbol, SYMBOL_UNTRAPPED_WRITE); /* avoid 
> recursion */
> +  ptrdiff_t count = SPECPDL_INDEX ();
> +  record_unwind_protect (&reset_symbol_trapped_write, symbol);
> +
> +  while (!NILP (watchers))
> +    {
> +      Lisp_Object watcher = XCAR (watchers);
> +      if (INTEGERP (watcher))
> +        {
> +          EMACS_INT wnum = XINT (watcher);
> +          if (wnum < ARRAYELTS (watcher_table))
> +            watcher_table[wnum] (operation, where, symbol, newval);
> +        }
> +      else if (FUNCTIONP (watcher))
> +        CALLN (Ffuncall, watcher, operation, where, symbol, newval);
> +      watchers = XCDR (watchers);
> +    }

The call to ARRAYELTS should be outside of the loop (for those poor
souls who run unoptimized builds most of the time).

>  static const WATCHER_FUNCTION watcher_table[] =
>    {
> +    &set_redisplay
>    };
>  enum
>    {
> +    WATCHER_NUMBER_SET_REDISPLAY
>    };

Shouldn't we make sure WATCHER_NUMBER_SET_REDISPLAY's value is zero?

> +  DEFVAR_INT ("set-redisplay-internal-watcher-number",
> +              Vset_redisplay_internal_watcher_number,
> +              doc: /* Internal watch function constant.  */);
> +  Vset_redisplay_internal_watcher_number = WATCHER_NUMBER_SET_REDISPLAY;
> +  make_symbol_constant (intern_c_string 
> ("set-redisplay-internal-watcher-number"));

I'd prefer if all this were moved to window.c.  data.c has no business
with display-related issues.

Please also add a short notice for etc/NEWS.  Bonus points for adding
some tests.

Other than that, and after the comments by others are taken care of, I
think this is good to go into master.

Thanks again for working on this.