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: Sun, 22 Feb 2015 00:32:02 +0000 Message-ID: <8vwOHsxYdm2iCzTC6gXmdGOk1vzQKZAhsD9XiWx1JDR@local> 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 1424565240 21360 80.91.229.3 (22 Feb 2015 00:34:00 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 22 Feb 2015 00:34:00 +0000 (UTC) Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sun Feb 22 01:33:49 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 1YPKUT-000505-Sn for ged-emacs-devel@m.gmane.org; Sun, 22 Feb 2015 01:33:46 +0100 Original-Received: from localhost ([::1]:38106 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPKUS-0007oo-TP for ged-emacs-devel@m.gmane.org; Sat, 21 Feb 2015 19:33:44 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:45382) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPKUO-0007oY-Hz for emacs-devel@gnu.org; Sat, 21 Feb 2015 19:33:41 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YPKUJ-0004EG-9E for emacs-devel@gnu.org; Sat, 21 Feb 2015 19:33:40 -0500 Original-Received: from relay4-d.mail.gandi.net ([2001:4b98:c:538::196]:52707) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPKUI-0004EA-WD for emacs-devel@gnu.org; Sat, 21 Feb 2015 19:33:35 -0500 Original-Received: from mfilter15-d.gandi.net (mfilter15-d.gandi.net [217.70.178.143]) by relay4-d.mail.gandi.net (Postfix) with ESMTP id B8823172080; Sun, 22 Feb 2015 01:33:33 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mfilter15-d.gandi.net Original-Received: from relay4-d.mail.gandi.net ([217.70.183.196]) by mfilter15-d.gandi.net (mfilter15-d.gandi.net [10.0.15.180]) (amavisd-new, port 10024) with ESMTP id hye4ZxzXe+7b; Sun, 22 Feb 2015 01:33:32 +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 relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 8B20F172070; Sun, 22 Feb 2015 01:33:28 +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::196 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:183375 Archived-At: Stefan Monnier wrote: > I definitely don't want magic constants, which is why I said "An `enum' > sounds right". If no magic constants, then all the places in trunk that have =E2=8C=9C->= constant =3D 1=E2=8C=9D must change to =E2=8C=9C->constant =3D SYM_CONST=E2= =8C=9D (and my patch already does this, but it changes the field name too= ). That's 9 lines of code. By retaining the name =E2=8C=9Cconstant=E2=8C=9D for the field name, I ca= n avoid touching 11 lines of code. But 9 of those are the aforementioned = lines, which means if magic constants aren't allowed, then retaining the = name =E2=8C=9Cconstant=E2=8C=9D saves changing just two lines of code. Th= at's a negligible reduction in the size of the patch, at the cost of reta= ining a field name that would no longer be appropriate. Leaving all 11 lines unchanged would leave us with both an inappropriate = field name and magic constants, and still be only a small reduction in th= e size of the patch. >>> What happens if we don't inline grow_specpdl? >> That's just an optimization since it's in the critical path of specbin= d, as >> I noted in my message when I made this change on Feb 9th. > > But that's orthogonal to the "variable hook" functionality, right? > So I think we can keep this for some later changes. You originally objected to my patch slowing down Emacs. So I looked for o= ptimization opportunities to ensure that my patch paid for its performanc= e costs. I was successful, and not only did I offset the costs, I even pr= oduced a net improvement in speed, and provided benchmarks to prove it. If I remove my optimizations, then my patch will slow down Emacs. And the= n you can once again object to the performance impact. And the above minor optimization is a trivial change in just two lines of= code, so removing it would produce a negligible reduction in the size of= the patch anyway. Removing the even-more-minor optimization that you ori= ginally insisted on (i.e. combining the constant and hooked fields to sav= e a conditional branch) would produce a much larger reduction in the size= of the patch, so if you're so worried about the patch size, how about we= remove the constant-hooked combination? My other optimizations, which to= uch fewer lines, actually improve the speed more than the constant-hooked= combination does. >> I didn't have to. It's just a minor optimization to avoid an extra >> conditional branch before the common case (SYMBOL_PLAINVAL) on the cri= tical > > Ah, you're moving the expected common case to the first position. > That sounds OK. But maybe we can keep this for some later changes, tho= . Same here. Just three occurrences of moving one line of code. And I have = to move one of those lines anyway, as part of the process of combining th= e constant and hooked fields into one field. > I can't imagine how a chunk of code you try to let-bind the symbol > that's the void-value, marking it "special" is pretty paranoid. Ok, I'll un-mark it as special. >> Because if you could, then as I explained in my previous message, you = could >> convert a setq into a makunbound, > > I don't think that's a serious problem. The tradeoff is providing a new capability (not previously present in Ema= cs) that's completely unnecessary (even for debugging) and pathological, = versus strictly abiding by the rule that hooking a symbol must not change= the behavior of Emacs. Either option is equally easy to implement, and t= he latter is obviously the right one. >> and since I changed setq to return the override value, > > By now, I think I've convinced myself that this an error. Ok, I'll change it back. Note that changing it back doesn't require enabl= ing conversion of setq into makunbound, so I'll still prevent the latter,= for the sake of correctness. > No, as I said, I'm happy to introduce such "bugs" for frame-local vars. Ok, if the alternative to intentionally introducing a bug is that my patc= h gets rejected, then I'll introduce the bug. But I'll add a comment that= it's an intentional bug.