From: Eli Zaretskii <eliz@gnu.org>
To: npostavs@users.sourceforge.net
Cc: 24923@debbugs.gnu.org
Subject: bug#24923: 25.1; Lisp watchpoints
Date: Fri, 11 Nov 2016 12:02:42 +0200 [thread overview]
Message-ID: <83eg2ie3lp.fsf@gnu.org> (raw)
In-Reply-To: <87vavun235.fsf@users.sourceforge.net> (npostavs@users.sourceforge.net)
> 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.
next prev parent reply other threads:[~2016-11-11 10:02 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 [this message]
2016-11-12 4:34 ` npostavs
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=83eg2ie3lp.fsf@gnu.org \
--to=eliz@gnu.org \
--cc=24923@debbugs.gnu.org \
--cc=npostavs@users.sourceforge.net \
/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.