From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Kelly Dean Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] (Updated) Run hook when variable is set Date: Sat, 21 Feb 2015 14:18:02 +0000 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 1424528388 13619 80.91.229.3 (21 Feb 2015 14:19:48 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 21 Feb 2015 14:19:48 +0000 (UTC) Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Feb 21 15:19:38 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 1YPAu9-0008M5-IS for ged-emacs-devel@m.gmane.org; Sat, 21 Feb 2015 15:19:37 +0100 Original-Received: from localhost ([::1]:36311 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPAu8-00057L-LY for ged-emacs-devel@m.gmane.org; Sat, 21 Feb 2015 09:19:36 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:35879) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPAu4-00057D-6h for emacs-devel@gnu.org; Sat, 21 Feb 2015 09:19:33 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YPAu0-00010d-U5 for emacs-devel@gnu.org; Sat, 21 Feb 2015 09:19:32 -0500 Original-Received: from relay6-d.mail.gandi.net ([2001:4b98:c:538::198]:57042) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPAu0-00010U-LI for emacs-devel@gnu.org; Sat, 21 Feb 2015 09:19:28 -0500 Original-Received: from mfilter5-d.gandi.net (mfilter5-d.gandi.net [217.70.178.132]) by relay6-d.mail.gandi.net (Postfix) with ESMTP id D2D62FB883; Sat, 21 Feb 2015 15:19:27 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mfilter5-d.gandi.net Original-Received: from relay6-d.mail.gandi.net ([217.70.183.198]) by mfilter5-d.gandi.net (mfilter5-d.gandi.net [10.0.15.180]) (amavisd-new, port 10024) with ESMTP id LsoeNgNVXRfV; Sat, 21 Feb 2015 15:19:26 +0100 (CET) X-Originating-IP: 66.220.3.179 Original-Received: from localhost (gm179.geneticmail.com [66.220.3.179]) (Authenticated sender: kelly@prtime.org) by relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 09B8DFB8A3; Sat, 21 Feb 2015 15:19:23 +0100 (CET) In-Reply-To: X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2001:4b98:c:538::198 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:183355 Archived-At: Stefan Monnier wrote: > I think either behavior is equally correct and wrong (sometimes you'd > rather want one and sometimes you'd rather want the other), so I'd > rather choose based on which of the two is more efficient/easy to > implement (and if it's not on the critical path (i.e. the path used whe= n > the variable is not hooked), then efficiency doesn't matter). It's not on the critical path, and they're equally efficient and easy to = implement. Returning the override value results in a few less lines of co= de (thus smaller patch), since in some places I don't have to put return = statements on separate lines from the calls to run_varhook, and in some p= laces that lets me eliminate a pair of curly brackets too. > In either case, `setq' should never return Qunbound. Right. It doesn't. > Regarding one of the bikesheds, [snip] > We can change the name later to something actually better, but for now, > let's just stick to "constant". Ok, I'll change it back. >> +#define SYM_CONST 1 >> +#define SYM_HOOKED 2 > constant and hooked at the same time is nonsensical, so these aren't tw= o > independent bits, instead `vetted' is a 3-valued field. An `enum' > sounds right. The compiler will encode the enum into Lisp_Symbol's bitfield, so that wi= ll produce no change in the compiled code. And SYM_CONST and SYM_HOOKED a= re better than sprinkling the code with 1 and 2 as magic numbers, for the= same reason that =C2=ABfalse=C2=BB and =C2=ABtrue=C2=BB are better than = 0 and 1 for booleans. But I'll put those constants into an enum. Note that those constant definitions have been there ever since I origina= lly combined the constant and hooked fields in the patch I submitted on F= eb 9th. >> -static void grow_specpdl (void); >> +static inline void grow_specpdl (void); > > What happens if we don't inline grow_specpdl? That's just an optimization since it's in the critical path of specbind, = as I noted in my message when I made this change on Feb 9th. The result i= s a small but measurable performance improvement, at the cost of increasi= ng the executable's size by a few kB (i.e. less than a tenth of a percent= , since Emacs is nearly 20MB already). If that's an undesirable tradeoff, I'll remove the inline. >> + case SYMBOL_VARALIAS: >> + sym =3D indirect_variable (sym); XSETSYMBOL (symbol, sym); goto= start; > > Why did you have to move this? [snip] > Hmm... same kind of change here. I must be missing something. I didn't have to. It's just a minor optimization to avoid an extra condit= ional branch before the common case (SYMBOL_PLAINVAL) on the critical pat= h, in case the compiler implements the switch statement as a series of =C2= =ABif=C2=BB, =C2=ABelse if=C2=BB, =C2=ABelse if=C2=BB statements. I decid= ed it was worth bothering to make this change because your original reaso= n for wanting to combine the constant and hooked fields was just to avoid= an extra conditional branch on this same critical path in set_internal. Same thing for the other two places (specbind and find_symbol_value) wher= e I made this same change. These have been here since Feb 9th, and on Feb 7th I explained why. >> + set_internal (symbol, old_value, where, true, Dyn_Unbind); > > If using "unbind" when referring to "undoing a let-binding" bothers you= , That doesn't bother me. =E2=80=9FUnbind=E2=80=9D is the right term for th= at. What bothers me is saying that a variable is =E2=80=9Funbound=E2=80=9D= when it's actually bound to void and the binding shadows a non-void bind= ing. > Personally I have no problem with using "unbind" > sometimes for "makunbound" and sometimes for "undo a let". Neither would I, if void could never shadow non-void. > I'd give it a more verbose name, to make sure it doesn't clash with > someone else's var. After all, it's not like this is going to be used > very often, so the length doesn't matter. `symbol-setter-void-value' > sounds fine. Ok, I'll change it. >> + XSYMBOL (Vvoid_sentinel)->declared_special =3D true; >> + XSYMBOL (Vvoid_sentinel)->vetted =3D SYM_CONST; > > This value is just a symbol, not a variable, so there's no point markin= g > it as special and/or constant. If the user does something stupid such as (set void-sentinel 'foo), it mi= ght as well barf. Especially since the print name looks like a keyword. T= hus marked constant. I don't see why mark it special, but t and nil are marked special, so I d= ecided to do this for the sake of consistency, in case there was some rea= son constants should be marked special. >> +buf-local: The setter's buffer-local environment. >> +dyn-local: The innermost dynamic environment in which SYMBOL is bound= . >> +dyn-bind: A new dynamic environment, such as creatable using `let'. > > I think this needs clarification, because when I read it, I don't know > for sure which one I'd get in which case. Ok, I'll elaborate. >> +If OLDVAL >> +is the value of void-sentinel but NEWVAL is not, you can override the= new >> +value, but you can't prevent the variable from being set to a non-voi= d value. > > Why not? Because if you could, then as I explained in my previous message, you cou= ld convert a setq into a makunbound, and since I changed setq to return t= he override value, that means setq could return Qunbound, which is someth= ing it should never do. >> +To hook all variables of a symbol, use `symbol-hook'. > > "all variables of a symbol" sounds really odd. Ok, I'll change it. > Whether there are > several variables for a single symbol is really a philosophical > question, Until you add multithreading to Emacs. ;-) > I'd much prefer to keep all the new functions/vars of this feature unde= r > a common (and new) prefix. I'll let you choose whether it should be > `symbol-setter-', `symbol-watcher-`, `symbol-hook-', or whatnot. Ok, I'll change it. > Why disallow Qunbound for all but makunbound? > I understand that it might be strange to return Qunbound in the `setq' > case, but I'm not sure it's worth the trouble trying to disallow it. Because I changed setq to return the override value. And as I noted, it a= lso has the fortunate side effect of making (setq x void-sentinel) behave= the same regardless of whether x is hooked. IOW, technically, allowing Q= unbound for setq would be a bug (even if setq returned its argument, inst= ead of the override value), because hooking a symbol must not change the = behavior of Emacs. > And it seems positively wrong in the case of unwinding the topmost let > binding of a variable, so even if we do want to disallow it, the test > shouldn't be "rawenv =3D=3D Dyn_Makvoid" but "orig_newval =3D=3D Qunbou= nd". > So, I think we don't need Dyn_Makvoid (hence we don't need to touch > Fmakunbound). Yup, that's a bug. Oops. > Since writing the above, I think I'm beginning to like the idea of > (setq ) always returning even if ends up set to > something else. It would be annoying in the case of (if (setq x y) ...), if you override = the setting of x but the =C2=ABif=C2=BB doesn't respect your choice. > Try M-: (list (setq auto-hscroll-mode 2) auto-hscroll-mode) RET Whether or not hooked, you get (2 t). If hooked, and you override to 3, y= ou get (3 t), not (t t), because the return value of setq is the override= value; setq doesn't set the variable and then read it back. >> - if (sym->constant) >> + if (SYM_CONST_P (sym)) >> error ("Symbol %s may not be frame-local", SDATA (SYMBOL_NAME (va= riable))); > > I think we can keep sym->constant here. That would be a bug, since it would signal an error if the symbol is hook= ed. Hooking a symbol must not change the behavior of Emacs. > we want to inflict pain on > the few rare remaining users of frame-local. Just deleting all the code for frame-local would accomplish that more eff= ectively.