* bug#5293: 23.1; unload-feature on buffer-local hooks @ 2010-01-02 21:06 Kevin Ryde 2010-01-02 23:14 ` Juanma Barranquero 2011-07-13 20:28 ` Juanma Barranquero 0 siblings, 2 replies; 21+ messages in thread From: Kevin Ryde @ 2010-01-02 21:06 UTC (permalink / raw) To: bug-gnu-emacs [-- Attachment #1: Type: text/plain, Size: 810 bytes --] When `unload-feature' looks in hooks for functions that it's going to unload, it doesn't seem to look in buffer-local values, other than for the current buffer. Evalling the code in try-foo.el below loads then unloads foo.el. It gets an error void-function foo-message where I hoped unload-feature might have purged that `foo-message' from `after-change-functions'. I suppose looking in all buffers is more work for unload-feature, but would be a good protection against bad things happening later. I expect some of the standard hooks like `after-change-functions' are used buffer-local most of the time. If instead it's an intentional omission (to save work) then the elisp manual and the docstring could note it so that modes or packages using buffer-local hook settings can take steps to undo. [-- Attachment #2: foo.el --] [-- Type: application/emacs-lisp, Size: 225 bytes --] [-- Attachment #3: try-foo.el --] [-- Type: application/emacs-lisp, Size: 197 bytes --] [-- Attachment #4: Type: text/plain, Size: 1077 bytes --] In GNU Emacs 23.1.1 (i486-pc-linux-gnu, GTK+ Version 2.16.5) of 2009-09-14 on raven, modified by Debian configured using `configure '--build=i486-linux-gnu' '--host=i486-linux-gnu' '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib' '--localstatedir=/var/lib' '--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes' '--enable-locallisppath=/etc/emacs23:/etc/emacs:/usr/local/share/emacs/23.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/23.1/site-lisp:/usr/share/emacs/site-lisp:/usr/share/emacs/23.1/leim' '--with-x=yes' '--with-x-toolkit=gtk' '--with-toolkit-scroll-bars' 'build_alias=i486-linux-gnu' 'host_alias=i486-linux-gnu' 'CFLAGS=-DDEBIAN -g -O2' 'LDFLAGS=-g' 'CPPFLAGS='' Important settings: value of $LC_ALL: nil value of $LC_COLLATE: nil value of $LC_CTYPE: nil value of $LC_MESSAGES: nil value of $LC_MONETARY: nil value of $LC_NUMERIC: nil value of $LC_TIME: nil value of $LANG: en_AU value of $XMODIFIERS: nil locale-coding-system: iso-latin-1-unix default-enable-multibyte-characters: t ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2010-01-02 21:06 bug#5293: 23.1; unload-feature on buffer-local hooks Kevin Ryde @ 2010-01-02 23:14 ` Juanma Barranquero 2011-07-13 20:28 ` Juanma Barranquero 1 sibling, 0 replies; 21+ messages in thread From: Juanma Barranquero @ 2010-01-02 23:14 UTC (permalink / raw) To: Kevin Ryde; +Cc: 5293 On Sat, Jan 2, 2010 at 22:06, Kevin Ryde <user42@zip.com.au> wrote: > When `unload-feature' looks in hooks for functions that it's going to > unload, it doesn't seem to look in buffer-local values, other than for > the current buffer. [...] > I suppose looking in all buffers is more work for unload-feature, but > would be a good protection against bad things happening later. Yes, unload-feature should look at buffer-local values, too. For the moment being, a package like foo should define a function (foo in the function name matches the feature name you're unloading): (defun foo-unload-function () ;; do whatever is necessary; in foo's case: (with-current-buffer (get-buffer "foo-bufer") (remove-hook 'after-change-functions 'foo-message t)) ;; continue standard unloading nil) that will be automatically called upon unloading foo.el; in most cases, it should return nil to allow unload-feature to continue the normal unloading process. Juanma ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2010-01-02 21:06 bug#5293: 23.1; unload-feature on buffer-local hooks Kevin Ryde 2010-01-02 23:14 ` Juanma Barranquero @ 2011-07-13 20:28 ` Juanma Barranquero 2011-07-15 0:26 ` Kevin Ryde 1 sibling, 1 reply; 21+ messages in thread From: Juanma Barranquero @ 2011-07-13 20:28 UTC (permalink / raw) To: Kevin Ryde; +Cc: 5293-done > Evalling the code in try-foo.el below loads then unloads foo.el. It > gets an error > > void-function foo-message > > where I hoped unload-feature might have purged that `foo-message' from > `after-change-functions'. > > I suppose looking in all buffers is more work for unload-feature, but > would be a good protection against bad things happening later. I expect > some of the standard hooks like `after-change-functions' are used > buffer-local most of the time. You're right that some hooks are used buffer-locally most of the time, but it's also true that in many cases they are used from a major mode, i.e., in your case, if foo.el defined a foo-mode and set `after-change-functions' locally in foo-mode buffers, unload-feature would do OK (with the current trunk, not any released Emacsen). As it is, foo.el is doing something non-standard, and unload-feature cannot try to revert by itself every non-standard thing packages do. That's why FEATURE-unload-function exists. So, in this case, the right fix would be adding this function to foo.el: (defun foo-unload-function () "Unload foo.el." (ignore-errors (with-current-buffer (get-buffer "foo-buffer") (remove-hook 'after-change-functions 'foo-message t))) ;; continue standard unloading nil) That said, there are improvements that could be made to unload-feature (for example, trying to automatically deactivate minor modes being undefined), but I'm not sure that looking in every buffer-local variable of every live buffer is a sensible thing to do. I'm closing this one because it is not really a bug. We can open a wishlist bug for unload-feature is you want. Juanma ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2011-07-13 20:28 ` Juanma Barranquero @ 2011-07-15 0:26 ` Kevin Ryde 2011-07-15 0:34 ` Juanma Barranquero 2011-07-16 18:50 ` Stefan Monnier 0 siblings, 2 replies; 21+ messages in thread From: Kevin Ryde @ 2011-07-15 0:26 UTC (permalink / raw) To: Juanma Barranquero; +Cc: 5293 Juanma Barranquero <lekktu@gmail.com> writes: > > but I'm not sure that looking in every buffer-local > variable of every live buffer No, just the hooks. If it makes sense to remove unloaded funcs from hook global values then surely the same rationale applies to remove them from the buffer-local values too. Or conversely, it's undesirable to leave behind an unbound func in a hook, and the same undesirability as to a buffer-local value as a global value. ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2011-07-15 0:26 ` Kevin Ryde @ 2011-07-15 0:34 ` Juanma Barranquero 2011-07-15 8:52 ` Štěpán Němec 2011-07-16 18:50 ` Stefan Monnier 1 sibling, 1 reply; 21+ messages in thread From: Juanma Barranquero @ 2011-07-15 0:34 UTC (permalink / raw) To: Kevin Ryde; +Cc: 5293 On Fri, Jul 15, 2011 at 02:26, Kevin Ryde <user42@zip.com.au> wrote: > Or conversely, it's undesirable to leave behind an unbound func in a > hook, and the same undesirability as to a buffer-local value as a global > value. But the usual case is that these buffer-local values are set via major modes also defined in the same package, and so they are automatically removed when the major modes are disabled (i.e., when the buffers are switched to other major modes). The only case where a buffer-local value is left behind is when the package's code sets it in non-standard ways, and in this case, it's the package responsability to define a FEATURE-unload-function to undo the changes. The philosophy behind unload-feature is: we try to automatically undo the easy/standard things, and give the package the opportunity to undo the hard/unstandard things itself. And I think it's the right approach. Juanma ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2011-07-15 0:34 ` Juanma Barranquero @ 2011-07-15 8:52 ` Štěpán Němec 2011-07-15 11:24 ` Juanma Barranquero 0 siblings, 1 reply; 21+ messages in thread From: Štěpán Němec @ 2011-07-15 8:52 UTC (permalink / raw) To: Juanma Barranquero; +Cc: Kevin Ryde, 5293 Juanma Barranquero <lekktu@gmail.com> writes: > On Fri, Jul 15, 2011 at 02:26, Kevin Ryde <user42@zip.com.au> wrote: > >> Or conversely, it's undesirable to leave behind an unbound func in a >> hook, and the same undesirability as to a buffer-local value as a global >> value. > > But the usual case is that these buffer-local values are set via major > modes also defined in the same package, and so they are automatically > removed when the major modes are disabled (i.e., when the buffers are > switched to other major modes). The only case where a buffer-local > value is left behind is when the package's code sets it in > non-standard ways, and in this case, it's the package responsability > to define a FEATURE-unload-function to undo the changes. > > The philosophy behind unload-feature is: we try to automatically undo > the easy/standard things, and give the package the opportunity to undo > the hard/unstandard things itself. And I think it's the right > approach. 1) If your reasoning about hooks being added via modes were correct, you wouldn't have to remove even the global hook additions. If it's faulty (which is probably the case), both global and local hooks need to be managed, as Kevin said. 2) The `unload-feature' docstring says it undoes "any additions that the library has made to hook variables", but that's apparently not what's really happening, so if things stay as they are, the doc string should be corrected. 3) Are local hook additions really such a "hard/unstandard" thing to undo? Štěpán ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2011-07-15 8:52 ` Štěpán Němec @ 2011-07-15 11:24 ` Juanma Barranquero 2011-07-15 16:08 ` Štěpán Němec 0 siblings, 1 reply; 21+ messages in thread From: Juanma Barranquero @ 2011-07-15 11:24 UTC (permalink / raw) To: Štěpán Němec; +Cc: Kevin Ryde, 5293 On Fri, Jul 15, 2011 at 10:52, Štěpán Němec <stepnem@gmail.com> wrote: > 1) If your reasoning about hooks being added via modes were correct, you > wouldn't have to remove even the global hook additions. Why? Global additions are not removed when the buffer's major mode is switched. Local variables, including local values of hooks, are. > If it's faulty > (which is probably the case), both global and local hooks need to be > managed, as Kevin said. I'm more of the opinion that both should be un-managed. A look at the sources is enough to see that hook removal is currently ugly and ad-hoc, and ugly too. > 2) The `unload-feature' docstring says it undoes "any additions that the > library has made to hook variables", but that's apparently not what's > really happening, so if things stay as they are, the doc string should > be corrected. Yes. The docstring for unload-feature has always promised more than it could reasonably accomplish. Yours is only one example. > 3) Are local hook additions really such a "hard/unstandard" thing to > undo? Local hook additions aren't. And they are correctly treated right now. What we are discussing is local hooks in buffers whose major mode wasn't also defined in the same feature being unloaded. For example, things like ;;; my-mode.el (define-derived-mode my-mode ...) (defun my-mode-this () ...) (defun my-mode-that () ...) (defun my-mode-hook () ...) (with-current-buffer (get-buffer "some poor unsuspecting buffer")) ;;; do not set the buffer major mode to my-mode, but (add-hook 'some-hook 'my-mode-hook nil t)) (provide 'my-mode) ;;;; end of my-mode.el and that kind of thing is unfrequent enough that the better fix is (defun my-mode-unload-function () (ignore-errors (with-current-buffer (get-buffer "some poor unsuspecting buffer") (remove-hook 'some-hook 'my-mode-hook t))) nil) Both Kevin and you seem to think that unload-feature should do everything and that having to define FEATURE-unload-function is a bug or something like that. It is not. I'm all for making unload-feature smarter, as long as it does not trample on the programmer's ability to do. I can perfectly well imagine unloading a package but setting a hook with an autoloading function from that same package. Juanma ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2011-07-15 11:24 ` Juanma Barranquero @ 2011-07-15 16:08 ` Štěpán Němec 2011-07-15 16:20 ` Juanma Barranquero 0 siblings, 1 reply; 21+ messages in thread From: Štěpán Němec @ 2011-07-15 16:08 UTC (permalink / raw) To: Juanma Barranquero; +Cc: Kevin Ryde, 5293 Juanma Barranquero <lekktu@gmail.com> writes: > On Fri, Jul 15, 2011 at 10:52, Štěpán Němec <stepnem@gmail.com> wrote: > >> 1) If your reasoning about hooks being added via modes were correct, you >> wouldn't have to remove even the global hook additions. > > Why? Global additions are not removed when the buffer's major mode is > switched. Local variables, including local values of hooks, are. Note I omitted the "major" part, i.e., it's not uncommon for minor modes to make global hook additions. Sorry if that's not really related to the problem at hand. >> If it's faulty >> (which is probably the case), both global and local hooks need to be >> managed, as Kevin said. > > I'm more of the opinion that both should be un-managed. A look at the > sources is enough to see that hook removal is currently ugly and > ad-hoc, and ugly too. Fine with me, as long as the documentation reflects this. >> 2) The `unload-feature' docstring says it undoes "any additions that the >> library has made to hook variables", but that's apparently not what's >> really happening, so if things stay as they are, the doc string should >> be corrected. > > Yes. The docstring for unload-feature has always promised more than it > could reasonably accomplish. Yours is only one example. Please do update it then. [...] > Both Kevin and you seem to think that unload-feature should do > everything and that having to define FEATURE-unload-function is a bug > or something like that. It is not. I'm all for making unload-feature > smarter, as long as it does not trample on the programmer's ability to > do. I can perfectly well imagine unloading a package but setting a > hook with an autoloading function from that same package. Right... I do know about unload functions and use them where appropriate. The important thing is that the documentation needs to describe what actually happens, so whatever you decide to do about this, please update the documentation (which, as you confirmed, needs to be done anyway). Štěpán ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2011-07-15 16:08 ` Štěpán Němec @ 2011-07-15 16:20 ` Juanma Barranquero 0 siblings, 0 replies; 21+ messages in thread From: Juanma Barranquero @ 2011-07-15 16:20 UTC (permalink / raw) To: Štěpán Němec; +Cc: Kevin Ryde, 5293 On Fri, Jul 15, 2011 at 18:08, Štěpán Němec <stepnem@gmail.com> wrote: > Note I omitted the "major" part, i.e., it's not uncommon for minor modes > to make global hook additions. Sorry if that's not really related to the > problem at hand. Currently, minor modes are not automatically turned off; packages that define minor modes *need* a FEATURE-unload-function. See allout.el, hi-lock.el, hl-line.el, linum.el, etc. Turning off the minor mode should remove these hooks. > The important thing is that the documentation needs to > describe what actually happens, so whatever you decide to do about this, > please update the documentation (which, as you confirmed, needs to be > done anyway). I agree that the documentation should better reflect what unload-feature actually does, but I won't be the one writing it. I suck at that. Juanma ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2011-07-15 0:26 ` Kevin Ryde 2011-07-15 0:34 ` Juanma Barranquero @ 2011-07-16 18:50 ` Stefan Monnier 2011-08-06 1:20 ` Kevin Ryde 1 sibling, 1 reply; 21+ messages in thread From: Stefan Monnier @ 2011-07-16 18:50 UTC (permalink / raw) To: Kevin Ryde; +Cc: Juanma Barranquero, 5293 > No, just the hooks. If it makes sense to remove unloaded funcs from > hook global values then surely the same rationale applies to remove them > from the buffer-local values too. Agreed. Someone would have to try it out to see if it can be done efficiently. Stefan ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2011-07-16 18:50 ` Stefan Monnier @ 2011-08-06 1:20 ` Kevin Ryde 2020-04-06 17:24 ` Štěpán Němec 0 siblings, 1 reply; 21+ messages in thread From: Kevin Ryde @ 2011-08-06 1:20 UTC (permalink / raw) To: control; +Cc: Juanma Barranquero, 5293 reopen 5293 thanks Stefan Monnier <monnier@iro.umontreal.ca> writes: > >> No, just the hooks. If it makes sense to remove unloaded funcs from >> hook global values then surely the same rationale applies to remove them >> from the buffer-local values too. > > Agreed. Someone would have to try it out to see if it can be > done efficiently. Reopened for that, or failing that then for clarifying the docstring. I imagine there's not normally many hooks or many buffers, or many buffer-local variables, whichever one of those was the efficient way to scan ... and unload-feature isn't used very much anyway. (Actually the way unload-feature seems not much used and not getting much attention from packages makes me wonder if it's worth bothering. But if unload-feature did a little more then it would increase its usefulness, perhaps to the point where it would be used more :-) ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2011-08-06 1:20 ` Kevin Ryde @ 2020-04-06 17:24 ` Štěpán Němec 2020-04-06 18:06 ` Stefan Monnier 2020-04-06 20:39 ` Juanma Barranquero 0 siblings, 2 replies; 21+ messages in thread From: Štěpán Němec @ 2020-04-06 17:24 UTC (permalink / raw) To: Kevin Ryde; +Cc: Juanma Barranquero, Stefan Monnier, 5293 [-- Attachment #1: Type: text/plain, Size: 1784 bytes --] On Sat, 06 Aug 2011 11:20:41 +1000 Kevin Ryde wrote: > reopen 5293 > thanks > > Stefan Monnier <monnier@iro.umontreal.ca> writes: >> >>> No, just the hooks. If it makes sense to remove unloaded funcs from >>> hook global values then surely the same rationale applies to remove them >>> from the buffer-local values too. >> >> Agreed. Someone would have to try it out to see if it can be >> done efficiently. ^^^^^^^^^^^ :-D > Reopened for that, or failing that then for clarifying the docstring. > > I imagine there's not normally many hooks or many buffers, or many > buffer-local variables, whichever one of those was the efficient way to > scan ... and unload-feature isn't used very much anyway. Coming back to this after 10 years, it appears that we (I, certainly) underestimated the computation involved. While the attached patch(es) work for emacs -Q toy examples like Kevin's or more recently the one from bug#34686, when I tried M-x load-library allout RET M-x unload-feature allout RET in my normal Emacs session with ~300 buffers and ~1000 features, it took my venerable laptop 8 minutes to complete. Based on that experience, unless someone has better ideas, I suggest we close this and clarify the (henceforth intentional) lack of attention to buffer-local hook values in the documentation instead. Actually, I wonder if ignoring even the global hooks (as opined by Juanma) and enforcing more widespread usage of FEATURE-unload-function wouldn't be better; or/also, couldn't stray undefined functions on hooks be handled similarly to how it's done for errors e.g. in `post-command-hook', i.e. auto-removed when encountered? I guess wrapping all hooks like that would be overkill? (That said, I think [1/3] and [3/3] could/should be applied nonetheless.) [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-unload-feature-Improve-logic-don-t-repeat-computatio.patch --] [-- Type: text/x-patch, Size: 2412 bytes --] From 394f2bf6821196a4171fd79fc9a10dc626619a58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0t=C4=9Bp=C3=A1n=20N=C4=9Bmec?= <stepnem@gmail.com> Date: Mon, 6 Apr 2020 13:25:41 +0200 Subject: [PATCH 1/3] unload-feature: Improve logic (don't repeat computation) * lisp/loadhist.el (unload-feature): Don't do the same computation twice. --- lisp/loadhist.el | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/lisp/loadhist.el b/lisp/loadhist.el index a1ff2f6270..60da00cceb 100644 --- a/lisp/loadhist.el +++ b/lisp/loadhist.el @@ -287,22 +287,23 @@ unload-feature ;; functions which the package might just have installed, and ;; there might be other important state, but this tactic ;; normally works. - (mapatoms - (lambda (x) - (when (and (boundp x) - (or (and (consp (symbol-value x)) ; Random hooks. - (string-match "-hooks?\\'" (symbol-name x))) - (memq x unload-feature-special-hooks))) ; Known abnormal hooks etc. - (dolist (y unload-function-defs-list) - (when (and (eq (car-safe y) 'defun) - (not (get (cdr y) 'autoload))) - (remove-hook x (cdr y))))))) - ;; Remove any feature-symbols from auto-mode-alist as well. - (dolist (y unload-function-defs-list) - (when (and (eq (car-safe y) 'defun) - (not (get (cdr y) 'autoload))) - (setq auto-mode-alist - (rassq-delete-all (cdr y) auto-mode-alist))))) + (let ((removables (cl-loop for def in unload-function-defs-list + when (and (eq (car-safe def) 'defun) + (not (get (cdr def) 'autoload))) + collect (cdr def)))) + (mapatoms + (lambda (x) + (when (and (boundp x) + (or (and (consp (symbol-value x)) ; Random hooks. + (string-match "-hooks?\\'" (symbol-name x))) + ;; Known abnormal hooks etc. + (memq x unload-feature-special-hooks))) + (dolist (func removables) + (remove-hook x func))))) + ;; Remove any feature-symbols from auto-mode-alist as well. + (dolist (func removables) + (setq auto-mode-alist + (rassq-delete-all func auto-mode-alist))))) ;; Change major mode in all buffers using one defined in the feature being unloaded. (unload--set-major-mode) -- 2.26.0 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: 0002-unload-feature-Handle-local-hooks.patch --] [-- Type: text/x-patch, Size: 1749 bytes --] From d2372157c031b79cad4e3bf331aa6e167ff48199 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0t=C4=9Bp=C3=A1n=20N=C4=9Bmec?= <stepnem@gmail.com> Date: Mon, 6 Apr 2020 13:30:11 +0200 Subject: [PATCH 2/3] unload-feature: Handle local hooks Buffer-local hooks were introduced in 1994-09-30T20:47:13+00:00!rms@gnu.org 0e4d378b32 (add-hook): Initialize default value and local value. but `unload-feature' has not been updated to handle them. * lisp/loadhist.el (unload-feature): Handle local hooks. --- etc/NEWS | 3 +++ lisp/loadhist.el | 6 +++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/etc/NEWS b/etc/NEWS index 765a923bf7..fa90edc4d1 100644 --- a/etc/NEWS +++ b/etc/NEWS @@ -297,6 +297,9 @@ optional argument specifying whether to follow symbolic links. ** 'parse-time-string' can now parse ISO 8601 format strings, such as "2020-01-15T16:12:21-08:00". +--- +** 'unload-feature' now also tries to undo additions to buffer-local hooks. + \f * Changes in Emacs 28.1 on Non-Free Operating Systems diff --git a/lisp/loadhist.el b/lisp/loadhist.el index 60da00cceb..2fd2d3f6be 100644 --- a/lisp/loadhist.el +++ b/lisp/loadhist.el @@ -299,7 +299,11 @@ unload-feature ;; Known abnormal hooks etc. (memq x unload-feature-special-hooks))) (dolist (func removables) - (remove-hook x func))))) + (remove-hook x func) + (save-current-buffer + (dolist (buffer (buffer-list)) + (set-buffer buffer) + (remove-hook x func t))))))) ;; Remove any feature-symbols from auto-mode-alist as well. (dolist (func removables) (setq auto-mode-alist -- 2.26.0 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #4: 0003-unload-feature-Correct-doc-string-to-match-info-manu.patch --] [-- Type: text/x-patch, Size: 2700 bytes --] From a418282b70e514065159a217a4106226395aa98e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0t=C4=9Bp=C3=A1n=20N=C4=9Bmec?= <stepnem@gmail.com> Date: Mon, 6 Apr 2020 17:05:33 +0200 Subject: [PATCH 3/3] unload-feature: Correct doc string to match info manual and reality 'unload-feature' doesn't try to "undo any additions the library has made" to hooks, it tries to remove functions defined by the library from hooks, no matter how they got there. * lisp/loadhist.el (unload-feature): Correct the doc string. * doc/lispref/loading.texi (Unloading): Clarify, fix typo. --- doc/lispref/loading.texi | 4 ++-- lisp/loadhist.el | 9 ++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/doc/lispref/loading.texi b/doc/lispref/loading.texi index 2894282079..f1ee638357 100644 --- a/doc/lispref/loading.texi +++ b/doc/lispref/loading.texi @@ -1063,7 +1063,7 @@ Unloading (Loading saves these in the @code{autoload} property of the symbol.) Before restoring the previous definitions, @code{unload-feature} runs -@code{remove-hook} to remove functions in the library from certain +@code{remove-hook} to remove functions defined by the library from certain hooks. These hooks include variables whose names end in @samp{-hook} (or the deprecated suffix @samp{-hooks}), plus those listed in @code{unload-feature-special-hooks}, as well as @@ -1071,7 +1071,7 @@ Unloading function because important hooks refer to functions that are no longer defined. -Standard unloading activities also undoes ELP profiling of functions +Standard unloading activities also undo ELP profiling of functions in that library, unprovides any features provided by the library, and cancels timers held in variables defined by the library. diff --git a/lisp/loadhist.el b/lisp/loadhist.el index 2fd2d3f6be..9718a4840d 100644 --- a/lisp/loadhist.el +++ b/lisp/loadhist.el @@ -234,11 +234,10 @@ unload-feature is nil, raise an error. Standard unloading activities include restoring old autoloads for -functions defined by the library, undoing any additions that the -library has made to hook variables or to `auto-mode-alist', undoing -ELP profiling of functions in that library, unproviding any features -provided by the library, and canceling timers held in variables -defined by the library. +functions defined by the library, removing such functions from +hooks and `auto-mode-alist', undoing their ELP profiling, +unproviding any features provided by the library, and canceling +timers held in variables defined by the library. If a function `FEATURE-unload-function' is defined, this function calls it with no arguments, before doing anything else. That function -- 2.26.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2020-04-06 17:24 ` Štěpán Němec @ 2020-04-06 18:06 ` Stefan Monnier 2020-04-06 19:17 ` Štěpán Němec 2020-04-06 20:39 ` Juanma Barranquero 1 sibling, 1 reply; 21+ messages in thread From: Stefan Monnier @ 2020-04-06 18:06 UTC (permalink / raw) To: Štěpán Němec; +Cc: Juanma Barranquero, Kevin Ryde, 5293 > @@ -299,7 +299,11 @@ unload-feature > ;; Known abnormal hooks etc. > (memq x unload-feature-special-hooks))) > (dolist (func removables) > - (remove-hook x func))))) > + (remove-hook x func) > + (save-current-buffer > + (dolist (buffer (buffer-list)) > + (set-buffer buffer) > + (remove-hook x func t))))))) > ;; Remove any feature-symbols from auto-mode-alist as well. > (dolist (func removables) > (setq auto-mode-alist Maybe instead of `(dolist (buffer (buffer-list))` within that big `mapatoms` within `(dolist (func removables)` (which is O(B*F*V) where B is the number of buffers, F is the number of functions and V is the number of hook vars), we should instead do it as: (dolist (buffer (buffer-list)) (dolist (varvar (buffer-local-variables buffer)) (when <var is a hook> (dolist (func removables) (remove-hook <var> func t))))) If we need it to go faster maybe we could also arrange for (add-hook V F ..) to do (cl-pushnew V (get F 'hooks-where-it-has-been-put)). So we could do (let ((relevant-hooks (mapcan (lambda (f) (get F 'hooks-where-it-has-been-put)) funcs))) (dolist (buffer (buffer-list)) (dolist (varvar (buffer-local-variables buffer)) (when (memq var relevant-hooks) (dolist (func removables) (remove-hook <var> func t))))) -- Stefan ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2020-04-06 18:06 ` Stefan Monnier @ 2020-04-06 19:17 ` Štěpán Němec 2020-09-30 18:44 ` Lars Ingebrigtsen 0 siblings, 1 reply; 21+ messages in thread From: Štěpán Němec @ 2020-04-06 19:17 UTC (permalink / raw) To: Stefan Monnier; +Cc: Juanma Barranquero, Kevin Ryde, 5293 [-- Attachment #1: Type: text/plain, Size: 1296 bytes --] On Mon, 06 Apr 2020 14:06:02 -0400 Stefan Monnier wrote: >> @@ -299,7 +299,11 @@ unload-feature >> ;; Known abnormal hooks etc. >> (memq x unload-feature-special-hooks))) >> (dolist (func removables) >> - (remove-hook x func))))) >> + (remove-hook x func) >> + (save-current-buffer >> + (dolist (buffer (buffer-list)) >> + (set-buffer buffer) >> + (remove-hook x func t))))))) >> ;; Remove any feature-symbols from auto-mode-alist as well. >> (dolist (func removables) >> (setq auto-mode-alist > > Maybe instead of `(dolist (buffer (buffer-list))` within that big > `mapatoms` within `(dolist (func removables)` (which is O(B*F*V) where > B is the number of buffers, F is the number of functions and V is the > number of hook vars), we should instead do it as: > > (dolist (buffer (buffer-list)) > (dolist (varvar (buffer-local-variables buffer)) ^^^^^^^^^^^^^^^^^^^^^^ Ha! Didn't know/think about that one, and indeed it makes a world of difference. Thanks! So I guess my defeatism will not prevail after all. Updated [2/3] attached. -- Štěpán [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-unload-feature-Handle-local-hooks.patch --] [-- Type: text/x-patch, Size: 1948 bytes --] From fd9b67b10fcc9596fc1eeae50bb351ef235eea68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0t=C4=9Bp=C3=A1n=20N=C4=9Bmec?= <stepnem@gmail.com> Date: Mon, 6 Apr 2020 13:30:11 +0200 Subject: [PATCH 2/3 v2] unload-feature: Handle local hooks Buffer-local hooks were introduced in 1994-09-30T20:47:13+00:00!rms@gnu.org 0e4d378b32 (add-hook): Initialize default value and local value. but `unload-feature' has not been updated to handle them. * lisp/loadhist.el (unload-feature): Handle local hooks. --- etc/NEWS | 3 +++ lisp/loadhist.el | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/etc/NEWS b/etc/NEWS index 765a923bf7..fa90edc4d1 100644 --- a/etc/NEWS +++ b/etc/NEWS @@ -297,6 +297,9 @@ optional argument specifying whether to follow symbolic links. ** 'parse-time-string' can now parse ISO 8601 format strings, such as "2020-01-15T16:12:21-08:00". +--- +** 'unload-feature' now also tries to undo additions to buffer-local hooks. + \f * Changes in Emacs 28.1 on Non-Free Operating Systems diff --git a/lisp/loadhist.el b/lisp/loadhist.el index 60da00cceb..81576679c3 100644 --- a/lisp/loadhist.el +++ b/lisp/loadhist.el @@ -300,6 +300,15 @@ unload-feature (memq x unload-feature-special-hooks))) (dolist (func removables) (remove-hook x func))))) + (save-current-buffer + (dolist (buffer (buffer-list)) + (pcase-dolist (`(,sym . ,val) (buffer-local-variables buffer)) + (when (or (and (consp val) + (string-match "-hooks?\\'" (symbol-name sym))) + (memq sym unload-feature-special-hooks)) + (set-buffer buffer) + (dolist (func removables) + (remove-hook sym func t)))))) ;; Remove any feature-symbols from auto-mode-alist as well. (dolist (func removables) (setq auto-mode-alist -- 2.26.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2020-04-06 19:17 ` Štěpán Němec @ 2020-09-30 18:44 ` Lars Ingebrigtsen 2020-10-20 10:20 ` Štěpán Němec 0 siblings, 1 reply; 21+ messages in thread From: Lars Ingebrigtsen @ 2020-09-30 18:44 UTC (permalink / raw) To: Štěpán Němec Cc: Juanma Barranquero, Kevin Ryde, Stefan Monnier, 5293 Štěpán Němec <stepnem@gmail.com> writes: > So I guess my defeatism will not prevail after all. Updated [2/3] attached. The patch no longer applied to the trunk, and it referred to a variable called `removables' that I couldn't find. So I've respun it (included below), but not tested. Any comments? diff --git a/etc/NEWS b/etc/NEWS index b4f29ab783..993cb3ca85 100644 --- a/etc/NEWS +++ b/etc/NEWS @@ -215,6 +215,9 @@ preserving markers, properties and overlays. The new variable number of seconds that 'revert-buffer-with-fine-grain' should spend trying to be non-destructive. +--- +** 'unload-feature' now also tries to undo additions to buffer-local hooks. + \f * Changes in Specialized Modes and Packages in Emacs 28.1 diff --git a/lisp/loadhist.el b/lisp/loadhist.el index a1ff2f6270..6f2e313f71 100644 --- a/lisp/loadhist.el +++ b/lisp/loadhist.el @@ -297,6 +297,16 @@ unload-feature (when (and (eq (car-safe y) 'defun) (not (get (cdr y) 'autoload))) (remove-hook x (cdr y))))))) + ;; Handle buffer-local hooks. + (save-current-buffer + (dolist (buffer (buffer-list)) + (pcase-dolist (`(,sym . ,val) (buffer-local-variables buffer)) + (when (or (and (consp val) + (string-match "-hooks?\\'" (symbol-name sym))) + (memq sym unload-feature-special-hooks)) + (set-buffer buffer) + (dolist (func unload-function-defs-list) + (remove-hook sym func t)))))) ;; Remove any feature-symbols from auto-mode-alist as well. (dolist (y unload-function-defs-list) (when (and (eq (car-safe y) 'defun) -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2020-09-30 18:44 ` Lars Ingebrigtsen @ 2020-10-20 10:20 ` Štěpán Němec 2020-10-20 11:13 ` Lars Ingebrigtsen 0 siblings, 1 reply; 21+ messages in thread From: Štěpán Němec @ 2020-10-20 10:20 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: Juanma Barranquero, Kevin Ryde, Stefan Monnier, 5293 [-- Attachment #1: Type: text/plain, Size: 928 bytes --] On Wed, 30 Sep 2020 20:44:04 +0200 Lars Ingebrigtsen wrote: > The patch no longer applied to the trunk, That's just the usual etc/NEWS conflict, I think. > and it referred to a variable called `removables' that I couldn't > find. It's a 3-patch series; the variable is introduced by the previous patch. For convenience, I've attached the whole series rebased on top of current master. Given the lack of further feedback, I haven't pushed (for pushing) this, as I kinda liked Stefan's other suggestion, too ("If we need it to go faster maybe we could also arrange for (add-hook V F ..) to do (cl-pushnew V (get F 'hooks-where-it-has-been-put)).", from https://lists.gnu.org/archive/html/bug-gnu-emacs/2020-04/msg00163.html jwvsghgcxi5.fsf-monnier+emacs@gnu.org ) But this version seems fast enough, so maybe we can push this and be done with it, at least for the time being. -- Štěpán [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-unload-feature-Improve-logic-don-t-repeat-computatio.patch --] [-- Type: text/x-patch, Size: 2412 bytes --] From 0d41a85c1881cd24a384e89227b3079bb62d5e30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0t=C4=9Bp=C3=A1n=20N=C4=9Bmec?= <stepnem@gmail.com> Date: Mon, 6 Apr 2020 13:25:41 +0200 Subject: [PATCH 1/3] unload-feature: Improve logic (don't repeat computation) * lisp/loadhist.el (unload-feature): Don't do the same computation twice. --- lisp/loadhist.el | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/lisp/loadhist.el b/lisp/loadhist.el index a1ff2f6270..60da00cceb 100644 --- a/lisp/loadhist.el +++ b/lisp/loadhist.el @@ -287,22 +287,23 @@ unload-feature ;; functions which the package might just have installed, and ;; there might be other important state, but this tactic ;; normally works. - (mapatoms - (lambda (x) - (when (and (boundp x) - (or (and (consp (symbol-value x)) ; Random hooks. - (string-match "-hooks?\\'" (symbol-name x))) - (memq x unload-feature-special-hooks))) ; Known abnormal hooks etc. - (dolist (y unload-function-defs-list) - (when (and (eq (car-safe y) 'defun) - (not (get (cdr y) 'autoload))) - (remove-hook x (cdr y))))))) - ;; Remove any feature-symbols from auto-mode-alist as well. - (dolist (y unload-function-defs-list) - (when (and (eq (car-safe y) 'defun) - (not (get (cdr y) 'autoload))) - (setq auto-mode-alist - (rassq-delete-all (cdr y) auto-mode-alist))))) + (let ((removables (cl-loop for def in unload-function-defs-list + when (and (eq (car-safe def) 'defun) + (not (get (cdr def) 'autoload))) + collect (cdr def)))) + (mapatoms + (lambda (x) + (when (and (boundp x) + (or (and (consp (symbol-value x)) ; Random hooks. + (string-match "-hooks?\\'" (symbol-name x))) + ;; Known abnormal hooks etc. + (memq x unload-feature-special-hooks))) + (dolist (func removables) + (remove-hook x func))))) + ;; Remove any feature-symbols from auto-mode-alist as well. + (dolist (func removables) + (setq auto-mode-alist + (rassq-delete-all func auto-mode-alist))))) ;; Change major mode in all buffers using one defined in the feature being unloaded. (unload--set-major-mode) -- 2.28.0 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: 0002-unload-feature-Handle-local-hooks.patch --] [-- Type: text/x-patch, Size: 1963 bytes --] From c50b6dec73739e5c3e1f18774b98adae213cf76c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0t=C4=9Bp=C3=A1n=20N=C4=9Bmec?= <stepnem@gmail.com> Date: Mon, 6 Apr 2020 13:30:11 +0200 Subject: [PATCH 2/3] unload-feature: Handle local hooks Buffer-local hooks were introduced in 1994-09-30T20:47:13+00:00!rms@gnu.org 0e4d378b32 (add-hook): Initialize default value and local value. but `unload-feature' has not been updated to handle them. * lisp/loadhist.el (unload-feature): Handle local hooks. --- etc/NEWS | 3 +++ lisp/loadhist.el | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/etc/NEWS b/etc/NEWS index c571fa95d1..3739b1c67e 100644 --- a/etc/NEWS +++ b/etc/NEWS @@ -1743,6 +1743,9 @@ to lexical binding, where dynamic (special) variables bound in one file can affect code in another. For details, see the manual section '(Elisp) Converting to Lexical Binding'. +--- +** 'unload-feature' now also tries to undo additions to buffer-local hooks. + \f * Changes in Emacs 28.1 on Non-Free Operating Systems diff --git a/lisp/loadhist.el b/lisp/loadhist.el index 60da00cceb..81576679c3 100644 --- a/lisp/loadhist.el +++ b/lisp/loadhist.el @@ -300,6 +300,15 @@ unload-feature (memq x unload-feature-special-hooks))) (dolist (func removables) (remove-hook x func))))) + (save-current-buffer + (dolist (buffer (buffer-list)) + (pcase-dolist (`(,sym . ,val) (buffer-local-variables buffer)) + (when (or (and (consp val) + (string-match "-hooks?\\'" (symbol-name sym))) + (memq sym unload-feature-special-hooks)) + (set-buffer buffer) + (dolist (func removables) + (remove-hook sym func t)))))) ;; Remove any feature-symbols from auto-mode-alist as well. (dolist (func removables) (setq auto-mode-alist -- 2.28.0 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #4: 0003-unload-feature-Correct-doc-string-to-match-info-manu.patch --] [-- Type: text/x-patch, Size: 2700 bytes --] From eb9a34d1e1e5527686a41561db5fd2797edb5074 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0t=C4=9Bp=C3=A1n=20N=C4=9Bmec?= <stepnem@gmail.com> Date: Mon, 6 Apr 2020 17:05:33 +0200 Subject: [PATCH 3/3] unload-feature: Correct doc string to match info manual and reality 'unload-feature' doesn't try to "undo any additions the library has made" to hooks, it tries to remove functions defined by the library from hooks, no matter how they got there. * lisp/loadhist.el (unload-feature): Correct the doc string. * doc/lispref/loading.texi (Unloading): Clarify, fix typo. --- doc/lispref/loading.texi | 4 ++-- lisp/loadhist.el | 9 ++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/doc/lispref/loading.texi b/doc/lispref/loading.texi index aa6ef307b1..e5364152d5 100644 --- a/doc/lispref/loading.texi +++ b/doc/lispref/loading.texi @@ -1063,7 +1063,7 @@ Unloading (Loading saves these in the @code{autoload} property of the symbol.) Before restoring the previous definitions, @code{unload-feature} runs -@code{remove-hook} to remove functions in the library from certain +@code{remove-hook} to remove functions defined by the library from certain hooks. These hooks include variables whose names end in @samp{-hook} (or the deprecated suffix @samp{-hooks}), plus those listed in @code{unload-feature-special-hooks}, as well as @@ -1071,7 +1071,7 @@ Unloading function because important hooks refer to functions that are no longer defined. -Standard unloading activities also undoes ELP profiling of functions +Standard unloading activities also undo ELP profiling of functions in that library, unprovides any features provided by the library, and cancels timers held in variables defined by the library. diff --git a/lisp/loadhist.el b/lisp/loadhist.el index 81576679c3..8ac575e8e3 100644 --- a/lisp/loadhist.el +++ b/lisp/loadhist.el @@ -234,11 +234,10 @@ unload-feature is nil, raise an error. Standard unloading activities include restoring old autoloads for -functions defined by the library, undoing any additions that the -library has made to hook variables or to `auto-mode-alist', undoing -ELP profiling of functions in that library, unproviding any features -provided by the library, and canceling timers held in variables -defined by the library. +functions defined by the library, removing such functions from +hooks and `auto-mode-alist', undoing their ELP profiling, +unproviding any features provided by the library, and canceling +timers held in variables defined by the library. If a function `FEATURE-unload-function' is defined, this function calls it with no arguments, before doing anything else. That function -- 2.28.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2020-10-20 10:20 ` Štěpán Němec @ 2020-10-20 11:13 ` Lars Ingebrigtsen 2020-10-21 17:00 ` Štěpán Němec 0 siblings, 1 reply; 21+ messages in thread From: Lars Ingebrigtsen @ 2020-10-20 11:13 UTC (permalink / raw) To: Štěpán Němec Cc: Juanma Barranquero, Kevin Ryde, Stefan Monnier, 5293 Štěpán Němec <stepnem@gmail.com> writes: > But this version seems fast enough, so maybe we can push this and be > done with it, at least for the time being. Yeah, I think so -- your patch looks like a good idea, and we can always tweak this more later, so please go ahead and push the patch series. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2020-10-20 11:13 ` Lars Ingebrigtsen @ 2020-10-21 17:00 ` Štěpán Němec 0 siblings, 0 replies; 21+ messages in thread From: Štěpán Němec @ 2020-10-21 17:00 UTC (permalink / raw) To: Lars Ingebrigtsen Cc: Juanma Barranquero, Kevin Ryde, Stefan Monnier, 5293-done On Tue, 20 Oct 2020 13:13:00 +0200 Lars Ingebrigtsen wrote: > Štěpán Němec <stepnem@gmail.com> writes: > >> But this version seems fast enough, so maybe we can push this and be >> done with it, at least for the time being. > > Yeah, I think so -- your patch looks like a good idea, and we can always > tweak this more later, so please go ahead and push the patch series. Done, thank you. 0e9e36747f (unload-feature: Improve logic (don't repeat computation)) 2020-04-06T13:25:41+02:00!stepnem@gmail.com 5c266a71c1 (unload-feature: Handle local hooks (bug#5293)) 2020-04-06T13:30:11+02:00!stepnem@gmail.com 8dc8ab6b42 (unload-feature: Correct doc string to match info manual and reality) 2020-04-06T17:05:33+02:00!stepnem@gmail.com -- Štěpán ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2020-04-06 17:24 ` Štěpán Němec 2020-04-06 18:06 ` Stefan Monnier @ 2020-04-06 20:39 ` Juanma Barranquero 2020-04-06 21:27 ` Štěpán Němec 1 sibling, 1 reply; 21+ messages in thread From: Juanma Barranquero @ 2020-04-06 20:39 UTC (permalink / raw) To: Štěpán Němec; +Cc: Kevin Ryde, Stefan Monnier, 5293 [-- Attachment #1: Type: text/plain, Size: 760 bytes --] On Mon, Apr 6, 2020 at 7:24 PM Štěpán Němec <stepnem@gmail.com> wrote: > Actually, I wonder if ignoring even the global hooks (as opined by > Juanma) and enforcing more widespread usage of FEATURE-unload-function > wouldn't be better; Anything automatically done in the unload-hook is just an ad hoc fix for things the module author knows how to do better than us. FEATURE-unload-function has already been there for a few years. I don't remember right now whether we suggest in the mode-creation documentation to use it, but certainly that's something module authors should do, and the automatic unloading is just a last-resort feature for those old modules that don't. There's no point IMHO to make the hands off approach work better. [-- Attachment #2: Type: text/html, Size: 1160 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2020-04-06 20:39 ` Juanma Barranquero @ 2020-04-06 21:27 ` Štěpán Němec 2020-04-06 23:01 ` Juanma Barranquero 0 siblings, 1 reply; 21+ messages in thread From: Štěpán Němec @ 2020-04-06 21:27 UTC (permalink / raw) To: Juanma Barranquero; +Cc: Kevin Ryde, Stefan Monnier, 5293 On Mon, 06 Apr 2020 22:39:50 +0200 Juanma Barranquero wrote: > On Mon, Apr 6, 2020 at 7:24 PM Štěpán Němec <stepnem@gmail.com> wrote: > >> Actually, I wonder if ignoring even the global hooks (as opined by >> Juanma) and enforcing more widespread usage of FEATURE-unload-function >> wouldn't be better; > > Anything automatically done in the unload-hook is just an ad hoc fix > for things the module author knows how to do better than us. One common scenario where this doesn't quite hold IIUC is minor modes which users are supposed to put on various hooks themselves: the library author has no way of dealing with that, short of doing something like `unload-feature' does, although checking for just a few known symbols from an unload function instead of the brute-force approach of the latter would arguably be more effective. > FEATURE-unload-function has already been there for a few years. I > don't remember right now whether we suggest in the mode-creation > documentation to use it, We do (lispref/tips.texi). Unfortunately, most elisp libraries in the wild seem to be written by people who either haven't read it, or have remained resistent to most of its edificatory influence. > but certainly that's something module authors should do, and the > automatic unloading is just a last-resort feature for those old > modules that don't. Actually, IME the older, the better behaved. I can't remember last time I saw a newish package with an unload function (while I do remember those without one which left my Emacs broken when I tried unloading them). > There's no point IMHO to make the hands off approach work better. I don't know what you mean by "hands off" here, but in any case, while I used to argue for handling as much as possible in `unload-feature', these days I don't feel strongly about it. So even though this particular issue (local hooks) does seem solvable (thanks again to Stefan!) without making anything much worse or uglier than it already is, I remain of two minds on whether it is the best thing to do or not. -- Štěpán ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#5293: 23.1; unload-feature on buffer-local hooks 2020-04-06 21:27 ` Štěpán Němec @ 2020-04-06 23:01 ` Juanma Barranquero 0 siblings, 0 replies; 21+ messages in thread From: Juanma Barranquero @ 2020-04-06 23:01 UTC (permalink / raw) To: Štěpán Němec; +Cc: Kevin Ryde, Stefan Monnier, 5293 [-- Attachment #1: Type: text/plain, Size: 2603 bytes --] On Mon, Apr 6, 2020 at 11:27 PM Štěpán Němec <stepnem@gmail.com> wrote: > One common scenario where this doesn't quite hold IIUC is minor modes > which users are supposed to put on various hooks themselves: the library > author has no way of dealing with that, short of doing something like > `unload-feature' does, although checking for just a few known symbols > from an unload function instead of the brute-force approach of the > latter would arguably be more effective. Some minor modes are designed to be put in a few hooks, so these can be checked by the package itself. But you're right that there are occasions where the automatic mechanisms are necessary. > We do (lispref/tips.texi). Unfortunately, most elisp libraries in the > wild seem to be written by people who either haven't read it, or have > remained resistent to most of its edificatory influence. In these cases, I would consider any problem unloading the library as a reportable bug. One good thing of FEATURE-unload-function is that it is totally backwards-compatible. Any library can define such a function and does not need to worry about being run in older Emacsen. > Actually, IME the older, the better behaved. I can't remember last time > I saw a newish package with an unload function (while I do remember > those without one which left my Emacs broken when I tried unloading > them). Well, I think what happens is that many package developers just don't think at all about the package being unloaded. Truth be told, I'm not sure unload-feature is used that often. I spent some effort in making it work with the libraries included in the Emacs distribution, but I think I've used unload-feature myself perhaps a couple of times, other than when testing it. > I don't know what you mean by "hands off" here, I meant the case where the package maintainer isn't willing to make the effort to add FEATURE-unload-function. > but in any case, while I > used to argue for handling as much as possible in `unload-feature', > these days I don't feel strongly about it. So even though this > particular issue (local hooks) does seem solvable (thanks again to > Stefan!) without making anything much worse or uglier than it already > is, I remain of two minds on whether it is the best thing to do or not. Well, I'm not against making unload-feature work better, if at all possible. It's just that I see as an imperfect solution because the knowledge to unload a package is, mostly, in the package author's hands. It's somewhat of a waste to have to second-guess them. [-- Attachment #2: Type: text/html, Size: 3098 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2020-10-21 17:00 UTC | newest] Thread overview: 21+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-01-02 21:06 bug#5293: 23.1; unload-feature on buffer-local hooks Kevin Ryde 2010-01-02 23:14 ` Juanma Barranquero 2011-07-13 20:28 ` Juanma Barranquero 2011-07-15 0:26 ` Kevin Ryde 2011-07-15 0:34 ` Juanma Barranquero 2011-07-15 8:52 ` Štěpán Němec 2011-07-15 11:24 ` Juanma Barranquero 2011-07-15 16:08 ` Štěpán Němec 2011-07-15 16:20 ` Juanma Barranquero 2011-07-16 18:50 ` Stefan Monnier 2011-08-06 1:20 ` Kevin Ryde 2020-04-06 17:24 ` Štěpán Němec 2020-04-06 18:06 ` Stefan Monnier 2020-04-06 19:17 ` Štěpán Němec 2020-09-30 18:44 ` Lars Ingebrigtsen 2020-10-20 10:20 ` Štěpán Němec 2020-10-20 11:13 ` Lars Ingebrigtsen 2020-10-21 17:00 ` Štěpán Němec 2020-04-06 20:39 ` Juanma Barranquero 2020-04-06 21:27 ` Štěpán Němec 2020-04-06 23:01 ` Juanma Barranquero
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.