From: npostavs@users.sourceforge.net
To: Eli Zaretskii <eliz@gnu.org>
Cc: 24923@debbugs.gnu.org
Subject: bug#24923: 25.1; Lisp watchpoints
Date: Fri, 11 Nov 2016 23:34:33 -0500 [thread overview]
Message-ID: <87pom1mi3q.fsf@users.sourceforge.net> (raw)
In-Reply-To: <83eg2ie3lp.fsf@gnu.org> (Eli Zaretskii's message of "Fri, 11 Nov 2016 12:02:42 +0200")
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'.)
next prev parent reply other threads:[~2016-11-12 4:34 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-11 3:10 bug#24923: 25.1; Lisp watchpoints npostavs
2016-11-11 10:02 ` Eli Zaretskii
2016-11-12 4:34 ` npostavs [this message]
2016-11-12 7:19 ` Eli Zaretskii
2016-11-13 0:54 ` npostavs
2016-11-13 15:29 ` Eli Zaretskii
2016-11-20 2:12 ` npostavs
2016-11-20 10:49 ` Stephen Berman
2016-11-20 14:14 ` npostavs
2016-11-20 16:11 ` Eli Zaretskii
2016-11-20 19:26 ` npostavs
2016-11-20 19:36 ` Eli Zaretskii
2016-11-20 20:16 ` npostavs
2016-11-21 17:31 ` Eli Zaretskii
2016-12-03 1:47 ` npostavs
2016-12-03 3:49 ` Clément Pit--Claudel
2016-12-03 3:50 ` Clément Pit--Claudel
2016-12-03 5:01 ` Daniel Colascione
2016-12-03 14:11 ` npostavs
2016-11-20 15:58 ` Eli Zaretskii
2016-11-20 17:00 ` npostavs
2016-11-20 17:25 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87pom1mi3q.fsf@users.sourceforge.net \
--to=npostavs@users.sourceforge.net \
--cc=24923@debbugs.gnu.org \
--cc=eliz@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.