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: Fri, 13 Feb 2015 19:55:18 -0500 Message-ID: References: NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Trace: ger.gmane.org 1423875355 11183 80.91.229.3 (14 Feb 2015 00:55:55 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 14 Feb 2015 00:55:55 +0000 (UTC) Cc: emacs-devel@gnu.org To: Kelly Dean Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Feb 14 01:55:46 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 1YMR1N-00053b-BO for ged-emacs-devel@m.gmane.org; Sat, 14 Feb 2015 01:55:45 +0100 Original-Received: from localhost ([::1]:58380 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YMR1M-0007pk-CS for ged-emacs-devel@m.gmane.org; Fri, 13 Feb 2015 19:55:44 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:51359) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YMR12-0007lu-QP for emacs-devel@gnu.org; Fri, 13 Feb 2015 19:55:26 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YMR0z-0006mE-F9 for emacs-devel@gnu.org; Fri, 13 Feb 2015 19:55:24 -0500 Original-Received: from pruche.dit.umontreal.ca ([132.204.246.22]:58510) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YMR0z-0006lw-AH for emacs-devel@gnu.org; Fri, 13 Feb 2015 19:55:21 -0500 Original-Received: from fmsmemgm.homelinux.net (lechon.iro.umontreal.ca [132.204.27.242]) by pruche.dit.umontreal.ca (8.14.1/8.14.1) with ESMTP id t1E0tIh0031215; Fri, 13 Feb 2015 19:55:18 -0500 Original-Received: by fmsmemgm.homelinux.net (Postfix, from userid 20848) id 7D227AE13C; Fri, 13 Feb 2015 19:55:18 -0500 (EST) In-Reply-To: (Kelly Dean's message of "Fri, 13 Feb 2015 23:08:21 +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 RV5216=0 X-NAI-Spam-Version: 2.3.0.9393 : core <5216> : inlines <2179> : streams <1389829> : uri <1854709> 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:183037 Archived-At: > The docstring for symbol-setter-function says lexical binding is required > for the hook function, I don't think that's true, BTW. I agree it's recommended (I recommend using lexical-binding for all new code), but other than that it should work just fine with dynamically scoped code. > If the hook function does set a dynamic variable that's hooked, and > has no terminating condition for the recursion, you'll immediately > find out when it exceeds max-lisp-eval-depth. So, don't do that. ;-) That's right. > So the user can notice, during debugging, if setq is setting the symbol > in a different environment than the one he intended, OK, so it's just informative, thanks. >> Hmm, no symbol-setter-function should be a variable (holding >> a function), modified via add-function/remove-function. > I don't see why; the only difference is using add-function with an unquot= ed > variable name vs. using advice-add with a quoted function name. It also > makes the hook run a bit slower. So far our coding style is to use *-hook, *-functions, and *-function for places where we expect that it's normal to override/modify existing behavior, whereas add-advice/defadvice is restricted to "last recourse". > And it results in the help page for the function exposing the advice > mechanism's internal representation of the advice, rather than cleanly > showing e.g. =E2=8C=9C:before advice: `mywatcher'=E2=8C=9D. I agree that this is unfortunate, and I hope we can improve the *Help* for those cases. In the future we may also use `defgeneric' for such "extension points". > But anyway I changed it to what you want, IIUC. I hope I misunderstood. Thanks. > The docstring already says to use :before if you just need to watch > variables, It shouldn't either. This issue of which kind of advice to use is not specific to this variable, so there's no need to put it there. > Unless maybe you want to have multiple pieces of advice wrapped around > each other, all blocking/overriding attempts to set variables? That's the intention, yes. > + /* When masked with SYMBOL_CONSTANT_MASK, non-zero means symbol is > + constant, i.e. changing its value should signal an error. > + 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. */ Actually I don't see these as two independent elements combined into one. I see it as a single value with 3 levels: - any write you want - write vetted by Elisp code - no write at all We could even drop the last setting and implement it in Elisp via symbol-setter-function, except that it's currently used for variables where we'd rather not give Elisp the opportunity to allow writes. > +DEFUN ("void-p", Fvoid_p, Svoid_p, 1, UNEVALLED, 0, > + doc: /* Return t if ARG has no value. > +If ARG is a non-lexical variable, this is equivalent to > +(not (boundp (quote ARG))). > + > +Unlike `boundp', this function can also test a lexical variable. I doubt this works on lexical-vars in compiled code. Why do we need it? > +This is the only built-in Elisp function that does not return a value. > +Returning the result of this function enables any other function > +to avoid returning a value. So far we've been extra careful to try and make sure Qunbound doesn't escape into Elisp world. I don't think we want to reconsider this design choice, since adding this Qunbound as a "normal" value in Elisp will just create the need for another special "really unbound" value. Why do we need that? > + DEFVAR_LISP ("symbol-setter-function", Vsymbol_setter_function, [...] > +If the variable is currently void, OLDVAL will be void. Ah, right, I see. No, we don't want to do that. I see some alternatives: - one is we don't provide OLDVAL at all (let the Elisp code get it using `symbol-value' or `symbol-default' according to ENV, instead). - OLDVAL is either a list of one element containing the old value, or nil (when that old value is Qunbound). - Use a special value to represent "unbound", e.g. a special keyword like `::value--unbound::'. Of course, the same holds for NEWVAL except that the code can't get NEWVAL via `symbol-value', so for NEWVAL we can't choose option 1. > + Vsymbol_setter_function =3D > + list4 (Qclosure, list1 (Qt), list4 (Qsym, Qenv, Qoldval, Qnewval), Q= newval); You can make it Qnil here and set it to a real value in subr.el, so as to avoid having to hard-code the internal representation of a particular function. Oh, and I think we can keep the name `constant' rather than use the verbose `constant_or_hooked'. It's not perfect, but at least it reduces the patch's size and the number of lines that are too long. Stefan