bug#24923: 25.1; Lisp watchpoints

[npostavs]

Eli Zaretskii <eliz@gnu.org> writes:
> 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?

Will do for the next one.

>
>> >From 0507854b885681c2e57bb1c6cb97f898417bd99c Mon Sep 17 00:00:00 2001
>
> This changeset was attached twice, for some reason.

Oops, I was trying to insert all the attachments at once in gnus via
some text manipulation and I forgot to delete the first one I copied
from.

>
>> --- 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?

Hmm, I'm not sure why I did this.  I think this whole hunk can be
dropped (as long as SYMBOL_CONSTANT_P is preserved as suggested below).


>
>> +  (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?

I don't think it would be especially useful, but it's easy to do so.

>
>> +/* 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?

I suppose I took Stefan's advice to replace SYMBOL_CONSTANT_P with
SYMBOL_TRAPPED_WRITE_P too literally.  Although it's almost always the
case that C code checking if a symbol is constant wants to write to the
symbol, in which case it should be doing a 3-way switch (writable,
trapped write, or constant).  But there is one exception in Fmakunbound
(mentioned above), so I guess we may as well keep it.

>
>> +          (_ (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.

You mean examples of this this clause being used?  It was meant more as
a catchall in case some watch types were missed by the previous clauses.
It shouldn't really ever happen unless the debugger and watchpoint code
get out of sync.  Do you think it would be better to just signal an
error?  (although would signalling an error while the debugger is
invoked cause trouble?)

>
>> +;;;###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?

Hah, I initially called this debug-on-set
(https://lists.nongnu.org/archive/html/emacs-devel/2015-11/txtyjJDztIULG.txt),
and then you suggested debug-watch instead
(https://lists.nongnu.org/archive/html/emacs-devel/2015-11/msg02017.html).

An alias probably makes sense, how about debug-on-variable-change?
(since it catches some changes other than `set'.)