From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#24923: 25.1; Lisp watchpoints Date: Fri, 11 Nov 2016 12:02:42 +0200 Message-ID: <83eg2ie3lp.fsf@gnu.org> References: <87vavun235.fsf@users.sourceforge.net> Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1478858603 4516 195.159.176.226 (11 Nov 2016 10:03:23 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 11 Nov 2016 10:03:23 +0000 (UTC) Cc: 24923@debbugs.gnu.org To: npostavs@users.sourceforge.net Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Fri Nov 11 11:03:18 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c58fz-0000FK-MJ for geb-bug-gnu-emacs@m.gmane.org; Fri, 11 Nov 2016 11:03:15 +0100 Original-Received: from localhost ([::1]:51735 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c58g2-00016N-Q6 for geb-bug-gnu-emacs@m.gmane.org; Fri, 11 Nov 2016 05:03:18 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:54515) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c58fp-00014j-VV for bug-gnu-emacs@gnu.org; Fri, 11 Nov 2016 05:03:07 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c58fm-0002gM-Py for bug-gnu-emacs@gnu.org; Fri, 11 Nov 2016 05:03:06 -0500 Original-Received: from debbugs.gnu.org ([208.118.235.43]:37603) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1c58fm-0002g7-M1 for bug-gnu-emacs@gnu.org; Fri, 11 Nov 2016 05:03:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1c58fm-0006Ni-Bt for bug-gnu-emacs@gnu.org; Fri, 11 Nov 2016 05:03:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 11 Nov 2016 10:03:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 24923 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 24923-submit@debbugs.gnu.org id=B24923.147885857124513 (code B ref 24923); Fri, 11 Nov 2016 10:03:02 +0000 Original-Received: (at 24923) by debbugs.gnu.org; 11 Nov 2016 10:02:51 +0000 Original-Received: from localhost ([127.0.0.1]:53002 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c58fa-0006NJ-Te for submit@debbugs.gnu.org; Fri, 11 Nov 2016 05:02:51 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:35129) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c58fY-0006N6-Mn for 24923@debbugs.gnu.org; Fri, 11 Nov 2016 05:02:49 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c58fQ-0002XC-9p for 24923@debbugs.gnu.org; Fri, 11 Nov 2016 05:02:43 -0500 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:58228) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c58fQ-0002X6-60; Fri, 11 Nov 2016 05:02:40 -0500 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:1416 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1c58fO-0003RL-Tm; Fri, 11 Nov 2016 05:02:39 -0500 In-reply-to: <87vavun235.fsf@users.sourceforge.net> (npostavs@users.sourceforge.net) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:125605 Archived-At: > 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.