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: Sat, 21 Feb 2015 15:51:33 -0500 Message-ID: References: NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable X-Trace: ger.gmane.org 1424551940 20583 80.91.229.3 (21 Feb 2015 20:52:20 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 21 Feb 2015 20:52:20 +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 21 21:52:12 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 1YPH24-0001UT-09 for ged-emacs-devel@m.gmane.org; Sat, 21 Feb 2015 21:52:12 +0100 Original-Received: from localhost ([::1]:37362 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPH23-0005Cp-5s for ged-emacs-devel@m.gmane.org; Sat, 21 Feb 2015 15:52:11 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:50420) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPH1z-0005Bo-L8 for emacs-devel@gnu.org; Sat, 21 Feb 2015 15:52:09 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YPH1w-00012Y-CD for emacs-devel@gnu.org; Sat, 21 Feb 2015 15:52:07 -0500 Original-Received: from ironport2-out.teksavvy.com ([206.248.154.181]:23742) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPH1w-00012O-8g for emacs-devel@gnu.org; Sat, 21 Feb 2015 15:52:04 -0500 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: ArsTAPOG1lTO+LI//2dsb2JhbABbgwaDX4VTwGUEAgKBDUQBAQEBAQF8hA0BBAEnLyMFCws0EhQYDSSIOAjOIwEBCAIBH49FMweEKgWKJ4MRjCeOGoF5gUUiggIcgW4ggnMBAQE X-IPAS-Result: ArsTAPOG1lTO+LI//2dsb2JhbABbgwaDX4VTwGUEAgKBDUQBAQEBAQF8hA0BBAEnLyMFCws0EhQYDSSIOAjOIwEBCAIBH49FMweEKgWKJ4MRjCeOGoF5gUUiggIcgW4ggnMBAQE X-IronPort-AV: E=Sophos;i="5.09,536,1418101200"; d="scan'208";a="111109643" Original-Received: from 206-248-178-63.dsl.teksavvy.com (HELO pastel.home) ([206.248.178.63]) by ironport2-out.teksavvy.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 21 Feb 2015 15:52:03 -0500 Original-Received: by pastel.home (Postfix, from userid 20848) id C39421462; Sat, 21 Feb 2015 15:51:33 -0500 (EST) In-Reply-To: (Kelly Dean's message of "Sat, 21 Feb 2015 14:18:02 +0000") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.50 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 206.248.154.181 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:183367 Archived-At: >>> +#define SYM_CONST 1 >>> +#define SYM_HOOKED 2 >> constant and hooked at the same time is nonsensical, so these aren't two >> 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. I know. My objection is mostly against the comment(s). They're basically a left over from when you had 2 separate fields but are out-of-date w.r.t the current code. > And SYM_CONST and SYM_HOOKED are better than sprinkling the code with > 1 and 2 as magic numbers, for the same reason that =ABfalse=BB and =ABtru= e=BB > are better than 0 and 1 for booleans. I definitely don't want magic constants, which is why I said "An `enum' sounds right". > 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 Feb 9= th. I know. >>> -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. But that's orthogonal to the "variable hook" functionality, right? So I think we can keep this for some later changes. > 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 critic= al 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. > Neither would I, if void could never shadow non-void. It definitely can. It almost never does, tho. > I don't see why mark it special, but t and nil are marked special, so > I decided to do this for the sake of consistency, in case there was some > reason constants should be marked special. I'm not sure exactly why t and nil are marked special, but the effect it has is to prevent their use a lexical vars, and hence prevent their use as let-bound vars altogether. The "constant" bit prevents a dynamically scoped (let ((t )) ...) while the "special" bit prevents a lexically scoped (let ((t )) ...). The first would be dangerous, while the second would be perfectly safe, but it might be a bit confusing for the programmer who expects t and nil to always refer to the special predefined constants. 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. > Because if you could, then as I explained in my previous message, you cou= ld > convert a setq into a makunbound, I don't think that's a serious problem. > and since I changed setq to return the override value, By now, I think I've convinced myself that this an error. > It would be annoying in the case of (if (setq x y) ...), if you override = the > setting of x but the =ABif=BB doesn't respect your choice. It all depends on what you mean by "override the setting". In the above code, the usual intention is "compute y, test the result, and remember it in x for later reuse". If the setter function decides to put some other value into `x', that doesn't mean that the test should also return another value. Here's another case: (setq x (setq y )) vs (setq y (setq x )) if you hook `x' and change the value to which it's set, do you really think the programmer expect the above two expressions to behave differently? >> Try M-: (list (setq auto-hscroll-mode 2) auto-hscroll-mode) RET > Whether or not hooked, you get (2 t). That's not the point: the `setq' returned the value passed to it, and not the value to which the variable was set. This is not done with your variable hooks, but by some pre-existing code instead, but the principle is the same. And in that pre-existing similar situation we made the opposite choice to the one you're making now. >>> - if (sym->constant) >>> + if (SYM_CONST_P (sym)) >>> error ("Symbol %s may not be frame-local", SDATA (SYMBOL_NAME (variable= ))); >> I think we can keep sym->constant here. > That would be a bug, since it would signal an error if the symbol is > hooked. Hooking a symbol must not change the behavior of Emacs. No, as I said, I'm happy to introduce such "bugs" for frame-local vars. >> 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 effectively. I've done that in my local branch, and every new release reduces the support of frame-local vars to some extent. I haven't thought enough about when we should finally remove support for them altogether (maybe 25.1 is a bit early still), but in the mean time, there's no reason to support interaction between them and new features. Stefan