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, 20 Feb 2015 14:29:07 -0500 Message-ID: References: NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1424460608 16709 80.91.229.3 (20 Feb 2015 19:30:08 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 20 Feb 2015 19:30:08 +0000 (UTC) Cc: emacs-devel@gnu.org To: Kelly Dean Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Feb 20 20:29:53 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 1YOtGq-0000cJ-O2 for ged-emacs-devel@m.gmane.org; Fri, 20 Feb 2015 20:29:53 +0100 Original-Received: from localhost ([::1]:33910 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YOtGp-0001bm-Uo for ged-emacs-devel@m.gmane.org; Fri, 20 Feb 2015 14:29:51 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:44476) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YOtGc-0001bY-1G for emacs-devel@gnu.org; Fri, 20 Feb 2015 14:29:39 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YOtGX-00030b-V7 for emacs-devel@gnu.org; Fri, 20 Feb 2015 14:29:37 -0500 Original-Received: from ironport2-out.teksavvy.com ([206.248.154.181]:59991) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YOtGX-00030X-OC for emacs-devel@gnu.org; Fri, 20 Feb 2015 14:29:33 -0500 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: ArsTAPOG1lTO+LI//2dsb2JhbABbgwaDX4VTwGUEAgKBDUQBAQEBAQF8hA0BBAEnLyMFCws0EhQYDSSIOAjOIwEBAQcBAQEBHo94B4QqBYonjziDA4JGiFGBeYFFIoICHIFuIIJzAQEB X-IPAS-Result: ArsTAPOG1lTO+LI//2dsb2JhbABbgwaDX4VTwGUEAgKBDUQBAQEBAQF8hA0BBAEnLyMFCws0EhQYDSSIOAjOIwEBAQcBAQEBHo94B4QqBYonjziDA4JGiFGBeYFFIoICHIFuIIJzAQEB X-IronPort-AV: E=Sophos;i="5.09,536,1418101200"; d="scan'208";a="111036443" 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; 20 Feb 2015 14:29:32 -0500 Original-Received: by pastel.home (Postfix, from userid 20848) id 6271B138E; Fri, 20 Feb 2015 14:29:07 -0500 (EST) In-Reply-To: (Kelly Dean's message of "Fri, 20 Feb 2015 06:48:42 +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:183331 Archived-At: > blocking/overriding the variable setting: if you hook a variable and > override its setting, then the return value of set, setq, etc should be the > override value instead of the originally attempted value, so that the > override will cascade through e.g. (setq x (setq y foo)) and (if (setq > z foo) ...). 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 when the variable is not hooked), then efficiency doesn't matter). In either case, `setq' should never return Qunbound. > Any other changes I should make? Well, now that you ask: Regarding one of the bikesheds, I think "constant" is a better name than "vetted" because the name really doesn't matter that much, "vetted" is not very self-explanatory, and "constant" has the major advantage of making the patch more readable by eliminating "irrelevant" changes. We can change the name later to something actually better, but for now, let's just stick to "constant". > +/* These are the masks for the vetted field of Lisp_Symbol. > + Bit 0 stores the constant field. Bit 1 stores the hooked field. */ > +#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. > -static void grow_specpdl (void); > +static inline void grow_specpdl (void); What happens if we don't inline grow_specpdl? > + if (!sym->vetted) SET_SYMBOL_VAL (sym, value); > + else if (SYM_HOOKED_P (sym)) Please don't put the "then" branch on the same line as the test, this is contrary to our coding conventions. > + case SYMBOL_VARALIAS: > + sym = indirect_variable (sym); XSETSYMBOL (symbol, sym); goto start; Why did you have to move this? > + set_internal (symbol, old_value, where, true, Dyn_Unbind); If using "unbind" when referring to "undoing a let-binding" bothers you, you could use "Dyn_Unwind" to mean that it's set as part of unwinding the stack of bindings. Personally I have no problem with using "unbind" sometimes for "makunbound" and sometimes for "undo a let". > + DEFSYM (Qvoid_sentinel, "void-sentinel"); > + DEFVAR_LISP ("void-sentinel", Vvoid_sentinel, > + doc: /* Representation of voidness for hooked variables. > +The value of this constant is an uninterned Lisp symbol that represents void > +when passed to or returned from `symbol-setter-function'. */); > + Vvoid_sentinel = Fmake_symbol (build_string ("::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. > + XSYMBOL (Vvoid_sentinel)->declared_special = true; > + XSYMBOL (Vvoid_sentinel)->vetted = SYM_CONST; This value is just a symbol, not a variable, so there's no point marking it as special and/or constant. > +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. I think the info I'd want to get is: - does it affect only the current buffer, or does it affect the default-value? - is it a "set", or a "bind" or an "unbind"? Note that those 2 aspects are orthogonal, so maybe we'd want 2 args rather than 1. > +If you use overriding advice, your advice must return the value to which to > +set the variable. Don't talk about advice here, just talk about what the function needs to do. After all, it may be modified without using advice. > +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-void value. Why not? > +To hook all variables of a symbol, use `symbol-hook'. "all variables of a symbol" sounds really odd. Whether there are several variables for a single symbol is really a philosophical question, and usually people seem to prefer thinking that it's not the case. >+ DEFVAR_LISP ("symbol-setter-function", Vsymbol_setter_function, > +DEFUN ("symbol-hooked-p", Fsymbol_hooked_p, Ssymbol_hooked_p, 1, 1, 0, > +DEFUN ("symbol-hook", Fsymbol_hook, Ssymbol_hook, 1, 1, 0, > +DEFUN ("symbol-unhook", Fsymbol_unhook, Ssymbol_unhook, 1, 1, 0, I'd much prefer to keep all the new functions/vars of this feature under a common (and new) prefix. I'll let you choose whether it should be `symbol-setter-', `symbol-watcher-`, `symbol-hook-', or whatnot. > - case SYMBOL_VARALIAS: sym = indirect_variable (sym); goto start; > case SYMBOL_PLAINVAL: return SYMBOL_VAL (sym); > + case SYMBOL_VARALIAS: sym = indirect_variable (sym); goto start; Why was this change needed? > + if (rawenv == Dyn_Makvoid && EQ (newval, Vvoid_sentinel)) > + return Qunbound; /* Converting setq, etc to makunbound is prohibited. */ > + return newval; /* So void_sentinel is ignored except for makunbound. */ 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. 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 == Dyn_Makvoid" but "orig_newval == Qunbound". So, I think we don't need Dyn_Makvoid (hence we don't need to touch Fmakunbound). > - set_internal (symbol, newval, Qnil, 0); > - return newval; > + return set_internal (symbol, newval, Qnil, false, Dyn_Current); 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. After all, that's what we've been doing so far. Try M-: (list (setq auto-hscroll-mode 2) auto-hscroll-mode) RET > - case SYMBOL_VARALIAS: sym = indirect_variable (sym); goto start; > case SYMBOL_PLAINVAL: return SYMBOL_VAL (sym); > + case SYMBOL_VARALIAS: sym = indirect_variable (sym); goto start; Hmm... same kind of change here. I must be missing something. > - 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. If things don't work 100% for frame-local vars, it's a good thing because we want to inflict pain on the few rare remaining users of frame-local. Stefan