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: Thu, 05 Feb 2015 08:57:46 -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 1423144695 30508 80.91.229.3 (5 Feb 2015 13:58:15 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 5 Feb 2015 13:58:15 +0000 (UTC) Cc: emacs-devel@gnu.org To: Kelly Dean Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Feb 05 14:58:15 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 1YJMwh-0007aZ-7h for ged-emacs-devel@m.gmane.org; Thu, 05 Feb 2015 14:58:15 +0100 Original-Received: from localhost ([::1]:42014 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJMwb-0006OA-Ge for ged-emacs-devel@m.gmane.org; Thu, 05 Feb 2015 08:58:09 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:48187) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJMwO-0006O4-CT for emacs-devel@gnu.org; Thu, 05 Feb 2015 08:57:57 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJMwH-00058N-Hs for emacs-devel@gnu.org; Thu, 05 Feb 2015 08:57:56 -0500 Original-Received: from chene.dit.umontreal.ca ([132.204.246.20]:41742) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJMwH-000589-CO for emacs-devel@gnu.org; Thu, 05 Feb 2015 08:57:49 -0500 Original-Received: from pastel.home (lechon.iro.umontreal.ca [132.204.27.242]) by chene.dit.umontreal.ca (8.14.1/8.14.1) with ESMTP id t15DvkC8015075; Thu, 5 Feb 2015 08:57:46 -0500 Original-Received: by pastel.home (Postfix, from userid 20848) id 95B3AFB3; Thu, 5 Feb 2015 08:57:46 -0500 (EST) In-Reply-To: (Kelly Dean's message of "Thu, 05 Feb 2015 03:10:57 +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 RV5208=0 X-NAI-Spam-Version: 2.3.0.9393 : core <5208> : inlines <2034> : streams <1385175> : uri <1847058> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 132.204.246.20 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:182442 Archived-At: > I'm unclear on your reason for extending the =ABconstant=BB field to > include the =ABhooked=BB bit, rather than giving the latter its own name. I thought it was clear: so that the new feature does not cost anything as long as you don't use it. I.e. that extends existing tests of "constant is 0" to mean both "not constant" and "not hooked either". > Either way, a new bit is needed (I can't fit the meaning of =ABhooked=BB > into =ABconstant=BB's current two bits) Why not? Currently `constant' occupies two bits but is used as a boolean value. So there's a bit available right there. And if not, you can add a bit to that field. > Example usage: I don't understand your example: what's `varhook'? What's `hook'? What's `unhook'? > +INLINE void > +try_run_varhook (struct Lisp_Symbol* sym, bool buf_local, > + Dyn_Bind_Direction dir, Lisp_Object value) > +{ > + if (sym->hooked) > + run_varhook (sym, buf_local, dir, value); > +} The hook should be called *to perform the assignment*, rather than calling it after performing the assignment ourselves. And it should not use run_hook* but funcall (i.e. the hook should be manipulated in Elisp via `add-function' rather than via `add-hook'). > -extern Lisp_Object run_hook_with_args (ptrdiff_t nargs, Lisp_Object *arg= s, > - Lisp_Object (*funcall) > - (ptrdiff_t nargs, Lisp_Object *args)); > +extern Lisp_Object run_hook_with_args (ptrdiff_t, Lisp_Object *, > + Lisp_Object (*) (ptrdiff_t, Lisp_Object *)); What for? > if (!sym->constant) > - SET_SYMBOL_VAL (sym, value); > + { > + SET_SYMBOL_VAL (sym, value); > + try_run_varhook (sym, false, Dyn_Bind, value); > + } You just slowed down all the code even if the feature is not used. I think we can do better. > if (sym->redirect =3D=3D SYMBOL_PLAINVAL) > { > - SET_SYMBOL_VAL (sym, specpdl_old_value (specpdl_ptr)); > + Lisp_Object oldval =3D specpdl_old_value (specpdl_ptr); > + SET_SYMBOL_VAL (sym, oldval); > + try_run_varhook (sym, false, Dyn_Unbind, oldval); Same here. > +run_varhook (struct Lisp_Symbol* sym, bool buf_local, Dyn_Bind_Direction= dir, > + Lisp_Object value) > +{ > + Lisp_Object hook_and_args[4]; > + if (dir =3D=3D Dyn_Skip) /* From backtrace_eval_unrewind */ > + return; Hmm... didn't think of backtrace_eval_unrewind. Indeed, I think we don't want this to trigger a hook (well, maybe there are cases where we'd want this, but I don't think the current code can handle it correctly/safely). > + hook_and_args[0] =3D Qvarhook; > + XSETSYMBOL (hook_and_args[1], sym); > + switch (dir) > + { > + case Dyn_Current: > + { > + bool shadowed; > + if (buf_local) > + shadowed =3D let_shadows_buffer_binding_p (sym); > + else shadowed =3D let_shadows_global_binding_p (hook_and_args[1]); > + if (shadowed) hook_and_args[2] =3D Qdyn_local; > + else if (buf_local) hook_and_args[2] =3D Qbuf_local; > + else hook_and_args[2] =3D Qglobal; > + break; > + } > + case Dyn_Global: > + { > + if (let_shadows_global_binding_p (hook_and_args[1])) > + hook_and_args[2] =3D Qinvalid; > + else hook_and_args[2] =3D Qglobal; > + break; > + } > + case Dyn_Bind: hook_and_args[2] =3D Qdyn_bind; break; > + case Dyn_Unbind: hook_and_args[2] =3D Qdyn_unbind; break; > + default: emacs_abort (); > + } > + hook_and_args[3] =3D value; > + run_hook_with_args (4, hook_and_args, funcall_nil); I think I'd rather turn `dir' into a Lisp_Object (a symbol) and send this directly to the hook. And then, if/when needed we can try and make available to Elisp the functionality of things like let_shadows_*_binding_p. BTW in order to keep the performance cost of this patch to a minimum, we may end up reducing its precision. > + DEFSYM (Qvarhook, "varhook"); Ah, here it is. Please use a name more along the lines of Elisp style. E.g. `variable-watch-function'. > + defsubr (&Shook); > + defsubr (&Sunhook); > + defsubr (&Shookedp); Use less generic names. E.g. `variable-watch-set', `variable-watch-unset', and `variable-watch-p'. Stefan