From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] (Updated) Run hook when variable is set Date: Thu, 12 Feb 2015 14:58:13 -0500 Message-ID: References: NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1423771127 3383 80.91.229.3 (12 Feb 2015 19:58:47 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 12 Feb 2015 19:58:47 +0000 (UTC) Cc: emacs-devel@gnu.org To: Kelly Dean Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Feb 12 20:58:37 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 1YLzuH-0001k1-4k for ged-emacs-devel@m.gmane.org; Thu, 12 Feb 2015 20:58:37 +0100 Original-Received: from localhost ([::1]:51899 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLzuG-00051h-Jr for ged-emacs-devel@m.gmane.org; Thu, 12 Feb 2015 14:58:36 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:44074) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLzu2-00051c-5L for emacs-devel@gnu.org; Thu, 12 Feb 2015 14:58:23 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YLztx-0002JE-9G for emacs-devel@gnu.org; Thu, 12 Feb 2015 14:58:22 -0500 Original-Received: from pruche.dit.umontreal.ca ([132.204.246.22]:34034) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLztx-0002J0-4Z for emacs-devel@gnu.org; Thu, 12 Feb 2015 14:58:17 -0500 Original-Received: from pastel.home (lechon.iro.umontreal.ca [132.204.27.242]) by pruche.dit.umontreal.ca (8.14.1/8.14.1) with ESMTP id t1CJwDkL021920; Thu, 12 Feb 2015 14:58:13 -0500 Original-Received: by pastel.home (Postfix, from userid 20848) id 2F7B010AF; Thu, 12 Feb 2015 14:58:13 -0500 (EST) In-Reply-To: (Kelly Dean's message of "Mon, 09 Feb 2015 03:24:51 +0000") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.50 (gnu/linux) X-NAI-Spam-Flag: NO X-NAI-Spam-Threshold: 5 X-NAI-Spam-Score: 0 X-NAI-Spam-Rules: 1 Rules triggered RV5215=0 X-NAI-Spam-Version: 2.3.0.9393 : core <5215> : inlines <2153> : streams <1389164> : uri <1853568> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 132.204.246.22 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:182974 Archived-At: >> No, the idea was rather to do: >> >> (defun set-internal-1 (args) >> (block nil ; Because Elisp isn't CL >> (if (constant-or-hooked-p) >> (if (constant-p) >> (if (forbidden-p) >> (error "setting constant") >> (return)) >> (funcall symbol-watch-function ..args..)) >> (do-some-stuff) >> (and-many-more-lines-of-stuff) >> (set-some-variable)))) > Then the many more lines of stuff have to be copied into > symbol-watch-function. Those lines can't just be skipped; We don't really need to copy those lines, instead symbol-watch-function should somehow (in the "normal" case) call us back, and in way that doesn't trigger the (constant-or-hooked-p) check. Of course, for that "...args..." should have enough info that we can indeed "call it back" properly. > By default, hooking a symbol must not change the behavior of Emacs, > except for slowing it down. A hook handler can optionally change the > behavior, but just hooking the symbol must cause no change if the > handler doesn't. Agreed. > Updated patches attached. The first applies to 24.4. We don't want to install this in emacs-24, so only the trunk (aka "master") code is important in this respect. [ BTW, even for 30KB patches, I prefer them uncompressed, because I typically read them directly in Gnus. ] > + /* When masked with SYMBOL_CONSTANT_MASK, non-zero means symbol is > + constant, i.e. changing its value should signal an error. If the > + value is 3, then the var can be changed, but only by `defconst'. > + When masked with SYMBOL_HOOKED_MASK, non-zero means setting > + symbol will run varhook. These two fields are combined into one > + in order to optimize the fast path of unhooked non-constants by > + having only one conditional branch for that case. */ We don't need this special value 3 for defconst, not only because the code doesn't use it, but also since if we want that unimplemented functionality, we can now implement it using your new symbol-setter system. > +typedef enum > + { > + Dyn_Unbind = -1, > + Dyn_Current = 0, > + Dyn_Bind = 1, > + Dyn_Skip = 2, > + Dyn_Global = 3 > + } Dyn_Bind_Direction; In which sense is this a "direction"? > +/* Like set_internal but with direction argument to indicate whether this > + function call is due to a binding (1), an unbinding (-1), or neither (0). > + As special cases, a value of 2 is a flag to disable run_varhook so that > + varhooks aren't run during backtraces, and a value of 3 is a flag > + indicating that this function call is due to set_default, which allows > + run_varhook to distinguish beween the global and the dyn-local binding. */ Please use the Dyn_* names rather than the numerical constants in the comment. > + Return the result of symbol-setter-function. The variable will be set > + (by code that calls run_varhook) to that result, overriding the value to > + which the setter is attempting to set the variable. */ That's a good idea, to circumvent the question of how to not-trigger the hooked-p check recursively when the hook function calls the setter (tho the question partly remains, in case the hook function *accidentally* sets one of the hooked variables). It does mean that the hooks can't redirect the assignment elsewhere, but maybe it's a good thing anyway. > + bool shadowed; > + if (buf_local) > + shadowed = let_shadows_buffer_binding_p (sym); > + else shadowed = let_shadows_global_binding_p (symbol); Aka bool shadowed = (buf_local ? let_shadows_buffer_binding_p (sym) : let_shadows_global_binding_p (symbol)); > + if (shadowed) env = Qdyn_local; > + else if (buf_local) env = Qbuf_local; > + else env = Qglobal; Why does the hook need to know about those different cases? > + start: /* Just to avoid reindentation in the varhook patch */ "diff -bw" works just as well for me. > + (XFWDTYPE (innercontents))==Lisp_Fwd_Buffer_Obj, ^^ We like to put spaces around infix operators. > +DEFUN ("symbol-setter-function", Fsymbol_setter_function, Ssymbol_setter_function, 4, 4, 0, Hmm, no symbol-setter-function should be a variable (holding a function), modified via add-function/remove-function. Also the docstring should not recommend :override (which should be a rather rare case, the more useful cases are probably :before and :around), and in general it should just say how the function is called and what it should return, without giving a primer about how to modify such *-function variables, since we don't want to do that for every *-function variable. OTOH the docstring should document the different possible values of ENV and their respective meaning. Stefan