From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: Lisp watchpoints Date: Sun, 29 Nov 2015 18:43:46 +0200 Message-ID: <83egf8oiwt.fsf@gnu.org> References: <83wpt922dn.fsf@gnu.org> Reply-To: Eli Zaretskii NNTP-Posting-Host: plane.gmane.org X-Trace: ger.gmane.org 1448815465 3821 80.91.229.3 (29 Nov 2015 16:44:25 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 29 Nov 2015 16:44:25 +0000 (UTC) Cc: jwiegley@gmail.com, monnier@iro.umontreal.ca, emacs-devel@gnu.org To: Noam Postavsky Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sun Nov 29 17:44:14 2015 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1a355B-0003uT-7t for ged-emacs-devel@m.gmane.org; Sun, 29 Nov 2015 17:44:13 +0100 Original-Received: from localhost ([::1]:36919 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a355F-0003eP-BM for ged-emacs-devel@m.gmane.org; Sun, 29 Nov 2015 11:44:17 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:37626) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3550-0003eK-1e for emacs-devel@gnu.org; Sun, 29 Nov 2015 11:44:03 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a354w-0007jN-J7 for emacs-devel@gnu.org; Sun, 29 Nov 2015 11:44:01 -0500 Original-Received: from mtaout29.012.net.il ([80.179.55.185]:42640) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a354w-0007j4-6A for emacs-devel@gnu.org; Sun, 29 Nov 2015 11:43:58 -0500 Original-Received: from conversion-daemon.mtaout29.012.net.il by mtaout29.012.net.il (HyperSendmail v2007.08) id <0NYL003004XUN900@mtaout29.012.net.il> for emacs-devel@gnu.org; Sun, 29 Nov 2015 18:43:33 +0200 (IST) Original-Received: from HOME-C4E4A596F7 ([84.94.185.246]) by mtaout29.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0NYL002U454J8710@mtaout29.012.net.il>; Sun, 29 Nov 2015 18:43:33 +0200 (IST) In-reply-to: X-012-Sender: halo1@inter.net.il X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6.x X-Received-From: 80.179.55.185 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:195551 Archived-At: > Date: Sat, 28 Nov 2015 21:04:25 -0500 > From: Noam Postavsky > Cc: Stefan Monnier , John Wiegley , emacs-devel@gnu.org > > From c1e2e14097f4488384ea8ea3cab3cd51c41947eb Mon Sep 17 00:00:00 2001 > From: Noam Postavsky > 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.