unload-feature questions and thoughts

unload-feature questions and thoughts

[Juanma Barranquero]

 1.- Currently, `unload-feature' (from loadhist.el) does not expect
entries on `load-history' of the form (autoload . SYMBOL) or (defface
. SYMBOL). Unloading a package which defines either one produces
messages like

  Unexpected element (defface . myface) in load-history

which should be reserved, I think, to really unexpected items in the
load-history, not perfectly usual ones like those above.

AFAICS, there's no way to delete an existing face, so ignoring the
item is the best option (or, alternatively, giving a more significant
warning, like "Face MYFACE can not be unloaded"). Autoload entries,
OTOH, can be unloaded like a function; so I propose to commit the
attached patch.

 2.- `unload-feature' returns `load-history', but this is not
documented, and a PITA when you're using `unload-feature' in a context
that evaluates its result, like IELM. Should we document what it does
now, or (preferred) just force it to return nil?

 3.- A weirdness of load vs. autoload. Let's suppose we have a package
test.el, with:

  ;;;;; test.el ;;;;;
  (defun test-fun () (interactive) t)
  (provide 'test)
  ;;;;;;;;;;;;;;;;;;;

and we create an autoload for it:

  (autoload 'test-fun "test" nil t)

Now, if we do load test.el by using the autoload mechanism, i.e.,

  M-x test-fun RET

then the autoload is recorded:

  (get 'test-fun 'autoload) => ("test" nil t nil)

However, if we load test.el with `load' or `require', the autoload is
not recorded:

  (require 'test)
  (get 'test-fun 'autoload) => nil

Which is not very important, but affects the ability of
`unload-feature' to do its work: it cannot restore an autoload if it
is not recorded in the property list.

                    /L/e/k/t/u


Index: lisp/ChangeLog
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/ChangeLog,v
retrieving revision 1.10649
diff -u -2 -r1.10649 ChangeLog
--- lisp/ChangeLog	4 Feb 2007 17:29:50 -0000	1.10649
+++ lisp/ChangeLog	4 Feb 2007 17:50:33 -0000
@@ -1,2 +1,8 @@
+2007-02-04  Juanma Barranquero  <lekktu@gmail.com>
+
+	* loadhist.el (unload-feature): Process also `load-history'
+	entries of the form `(autoload . SYMBOL)' (by treating them like
+	`defun'), and `(defface . SYMBOL)' (by ignoring them).
+
 2007-02-04  David Kastrup  <dak@gnu.org>

Index: lisp/loadhist.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/loadhist.el,v
retrieving revision 1.43
diff -u -2 -r1.43 loadhist.el
--- lisp/loadhist.el	21 Jan 2007 03:53:11 -0000	1.43
+++ lisp/loadhist.el	4 Feb 2007 16:59:22 -0000
@@ -216,5 +216,5 @@
 	   (provide
 	    (setq features (delq (cdr x) features)))
-	   (defun
+	   ((defun autoload)
 	    (let ((fun (cdr x)))
 	      (when (fboundp fun)
@@ -225,5 +225,5 @@
                       (fset fun (cons 'autoload aload))
                     (fmakunbound fun))))))
-           ((t require) nil)
+           ((t require defface) nil)
 	   (t (message "Unexpected element %s in load-history" x)))
 	;; Kill local values as much as possible.

Re: unload-feature questions and thoughts

[David Kastrup]

"Juanma Barranquero" <lekktu@gmail.com> writes:

> 3.- A weirdness of load vs. autoload. Let's suppose we have a package
> test.el, with:
>
>  ;;;;; test.el ;;;;;
>  (defun test-fun () (interactive) t)
>  (provide 'test)
>  ;;;;;;;;;;;;;;;;;;;
>
> and we create an autoload for it:
>
>  (autoload 'test-fun "test" nil t)
>
> Now, if we do load test.el by using the autoload mechanism, i.e.,
>
>  M-x test-fun RET
>
> then the autoload is recorded:
>
>  (get 'test-fun 'autoload) => ("test" nil t nil)
>
> However, if we load test.el with `load' or `require', the autoload is
> not recorded:
>
>  (require 'test)
>  (get 'test-fun 'autoload) => nil
>
> Which is not very important, but affects the ability of
> `unload-feature' to do its work: it cannot restore an autoload if it
> is not recorded in the property list.

We had this discussion already.  IIRC, load could be used multiple
times and in so many contexts, that is was not reasonable to expect it
to be undoable.

As one consequence, AUCTeX typically uses a startup-file looking the
following:

;;; auctex.el
;;
;; This can be used for starting up AUCTeX.  The following somewhat
;; strange trick causes tex-site.el to be loaded in a way that can be
;; safely undone using (unload-feature 'tex-site).
;;
(autoload 'TeX-load-hack
  (expand-file-name "tex-site.el" (file-name-directory load-file-name)))
(TeX-load-hack)

and then we have

TeX-load-hack is an alias for `ignore' in `tex-site.el'.
(TeX-load-hack &rest IGNORE)

Do nothing and return nil.
This function accepts any number of arguments, but ignores them.

[back]

Before we coded this hack, there was a discussion on emacs-devel
because I had been surprised, too.

Anyway, this is most emphatically not something that should be changed
now.  But maybe it would be worth documenting something about it.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 2/4/07, David Kastrup <dak@gnu.org> wrote:

> We had this discussion already.  IIRC, load could be used multiple
> times and in so many contexts, that is was not reasonable to expect it
> to be undoable.

OTOH, `unload-feature' exists for a reason; and packages that do
non-standard things (like redefining standard functions) can define
their own unload functions.

But question 3) was not about unloading, though I mentioned it at the
end; it was about the difference in the resulting 'autoload property
of function symbols depending on whether you use the autoloading vs.
the normal load/require mechanisms.

> Before we coded this hack, there was a discussion on emacs-devel
> because I had been surprised, too.

Couldn't AUCTeX just define AUCTeX-unload-hook and do whatever it is necessary?

> Anyway, this is most emphatically not something that should be changed
> now.

Agreed. I didn't send a patch or even suggested a "fix" :)

> But maybe it would be worth documenting something about it.

I think so.
                    /L/e/k/t/u

Re: unload-feature questions and thoughts

[David Kastrup]

"Juanma Barranquero" <lekktu@gmail.com> writes:

> On 2/4/07, David Kastrup <dak@gnu.org> wrote:
>
>> We had this discussion already.  IIRC, load could be used multiple
>> times and in so many contexts, that is was not reasonable to expect it
>> to be undoable.
>
> OTOH, `unload-feature' exists for a reason; and packages that do
> non-standard things (like redefining standard functions) can define
> their own unload functions.
>
> But question 3) was not about unloading, though I mentioned it at the
> end; it was about the difference in the resulting 'autoload property
> of function symbols depending on whether you use the autoloading vs.
> the normal load/require mechanisms.
>
>> Before we coded this hack, there was a discussion on emacs-devel
>> because I had been surprised, too.
>
> Couldn't AUCTeX just define AUCTeX-unload-hook and do whatever it is
> necessary?

It defines tex-site-unload-hook already.  But "whatever is necessary"
pretty much means restoring all functions overwritten by AUCTeX, and
there is no point in not using the autoload unloader for that.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 2/4/07, David Kastrup <dak@gnu.org> wrote:

> It defines tex-site-unload-hook already.  But "whatever is necessary"
> pretty much means restoring all functions overwritten by AUCTeX, and
> there is no point in not using the autoload unloader for that.

In fact (and this is not a proposal, at least not now), the current
working of `unload-feature' is a bit puzzling. Instead of running
either `FEATURE-unload-hook' (if found) or its normal heuristics, it'd
be better if it ran a FEATURE-unload-function, which would return t to
stop, or nil to *also* use the function's current heuristics. That
way, a package could define a FEATURE-unload-function to undo unusual
things, and leave the burden of the unloading to `unload-feature'.

I imagine it could be faked right now with:

  (defun myfeature-unload-hook ()
   ;; Undo unusual things, then:
   (let ((fun (symbol-function 'myfeature-unload-hook)))
     (fmakunbound 'myfeature-unload-hook)
     (ignore-errors (unload-feature 'myfeature))
     (fset 'myfeature-unload-hook fun)))

which is quite ugly, but it should work.

                    /L/e/k/t/u

Re: unload-feature questions and thoughts

[David Kastrup]

"Juanma Barranquero" <lekktu@gmail.com> writes:

> On 2/4/07, David Kastrup <dak@gnu.org> wrote:
>
>> It defines tex-site-unload-hook already.  But "whatever is necessary"
>> pretty much means restoring all functions overwritten by AUCTeX, and
>> there is no point in not using the autoload unloader for that.
>
> In fact (and this is not a proposal, at least not now), the current
> working of `unload-feature' is a bit puzzling. Instead of running
> either `FEATURE-unload-hook' (if found) or its normal heuristics, it'd
> be better if it ran a FEATURE-unload-function, which would return t to
> stop, or nil to *also* use the function's current heuristics. That
> way, a package could define a FEATURE-unload-function to undo unusual
> things, and leave the burden of the unloading to `unload-feature'.

You are confused.  FEATURE-unload-hook only caters for removing
FEATURE-related functions from hook variables (if FEATURE-unload-hook
is not used, Emacs basically walks through all variables ending in
"-hook" and removes variables from FEATURE from them).  All the rest
are unloaded as normal unless FEATURE-unload-hook removes the
corresponding FEATURE from unload-hook-features-list.

-- 
David Kastrup

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 2/5/07, David Kastrup <dak@gnu.org> wrote:

> You are confused.

It often happens ;)

> FEATURE-unload-hook only caters for removing
> FEATURE-related functions from hook variables

You're right. I was misreading that part of the code.

                    /L/e/k/t/u

Re: unload-feature questions and thoughts

[David Kastrup]

"Juanma Barranquero" <lekktu@gmail.com> writes:

> On 2/5/07, David Kastrup <dak@gnu.org> wrote:
>
>> You are confused.
>
> It often happens ;)
>
>> FEATURE-unload-hook only caters for removing
>> FEATURE-related functions from hook variables
>
> You're right. I was misreading that part of the code.

Could you think of something in code or docs that should have
prevented the confusion?  I seem to remember that I actually had to
read the code before getting the right idea about what
FEATURE-unload-hook does.

-- 
David Kastrup

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 2/5/07, David Kastrup <dak@gnu.org> wrote:

> Could you think of something in code or docs that should have
> prevented the confusion?

I misread the code (I was much more centered in the part about undoing
defun, provide, etc. and didn't take a closer look to the beginning of
the function), but I think this piece of docstring is a bit confusing

  This function tries to undo modifications made by the package to
  hooks.  Packages may define a hook FEATURE-unload-hook that is called
  instead of the normal heuristics for doing this.  Such a hook should
  undo all the relevant global state changes that may have been made by
  loading the package or executing functions in it.

because if you miss the "to hooks" (two words), as I did in several
readings, it seems like it is saying that either you define
FEATURE-unload-hook, or else it uses "the normal heuristics".

                    /L/e/k/t/u

Re: unload-feature questions and thoughts

[David Kastrup]

"Juanma Barranquero" <lekktu@gmail.com> writes:

> On 2/5/07, David Kastrup <dak@gnu.org> wrote:
>
>> Could you think of something in code or docs that should have
>> prevented the confusion?
>
> I misread the code (I was much more centered in the part about undoing
> defun, provide, etc. and didn't take a closer look to the beginning of
> the function), but I think this piece of docstring is a bit confusing
>
>  This function tries to undo modifications made by the package to
>  hooks.  Packages may define a hook FEATURE-unload-hook that is called
>  instead of the normal heuristics for doing this.  Such a hook should
>  undo all the relevant global state changes that may have been made by
>  loading the package or executing functions in it.
>
> because if you miss the "to hooks" (two words), as I did in several
> readings, it seems like it is saying that either you define
> FEATURE-unload-hook, or else it uses "the normal heuristics".

Maybe "the normal heuristics" and "all the relevant global state
changes" are far too much hand-waving: readers will probably
phantasize whatever they would think useful into it.

-- 
David Kastrup

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 2/5/07, David Kastrup <dak@gnu.org> wrote:

> Maybe "the normal heuristics" and "all the relevant global state
> changes" are far too much hand-waving: readers will probably
> phantasize whatever they would think useful into it.

Perhaps. Care to suggest better wording?

                    /L/e/k/t/u

Re: unload-feature questions and thoughts

[Richard Stallman]

    the function), but I think this piece of docstring is a bit confusing

      This function tries to undo modifications made by the package to
      hooks.  Packages may define a hook FEATURE-unload-hook that is called
      instead of the normal heuristics for doing this.  Such a hook should
      undo all the relevant global state changes that may have been made by
      loading the package or executing functions in it.

    because if you miss the "to hooks" (two words), as I did in several
    readings, it seems like it is saying that either you define
    FEATURE-unload-hook, or else it uses "the normal heuristics".

I agree, let's clarify this -- in the doc string and in the Lisp
manual.  Would someone like to try it?

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 2/5/07, Juanma Barranquero <lekktu@gmail.com> wrote:

> I misread the code (I was much more centered in the part about undoing
> defun, provide, etc. and didn't take a closer look to the beginning of
> the function), but I think this piece of docstring is a bit confusing
>
>   This function tries to undo modifications made by the package to
>   hooks.  Packages may define a hook FEATURE-unload-hook that is called
>   instead of the normal heuristics for doing this.  Such a hook should
>   undo all the relevant global state changes that may have been made by
>   loading the package or executing functions in it.
>
> because if you miss the "to hooks" (two words), as I did in several
> readings, it seems like it is saying that either you define
> FEATURE-unload-hook, or else it uses "the normal heuristics".

Eight months later, I had forgotten this thread and I *again* misread
both the code and the docstring. :(

So, could someone with good English skills please reword
unload-feature's docstring to clarify what it does and what purpose
FEATURE-unload-hook does serve?

Related to this: I'm still unsatisfied with FEATURE-unload-hook. It is
not useful enough when you want do undo other kind of changes. For
example, currently follow.el can not be correctly unloaded because it
uses tons of advices. It is possible to define a follow-unload-hook to
undo the advices, but then it has to take care of removing functions
from hooks, too.

Some packages with a global effect, like server.el or msb.el, run
(MODE -1) in the FEATURE-unload-hook to disable the hooks, and then do
the additional cleanup; but this is less practical for non-global
modes like follow-mode.

All in all, I think it'd be useful to have a FEATURE-unload-function
which would return t or nil to signal whether it has done everything,
or would like for unload-feature to continue with its normal
processing.

             Juanma

Re: unload-feature questions and thoughts

[Davis Herring]

> Eight months later, I had forgotten this thread and I *again* misread
> both the code and the docstring. :(
>
> So, could someone with good English skills please reword
> unload-feature's docstring to clarify what it does and what purpose
> FEATURE-unload-hook does serve?

I'm glad to contribute a native speaker's touch, but I'm afraid that I
don't see what's wrong with it.  Looking at the code, it does seem that it
invokes that hook ("fortran-unload-hook" for (unload-feature 'fortran)) if
it exists, or else goes hunting for non-autoloaded functions defined by
the package that are on -hook/etc. variables or on `auto-mode-alist'.  Am
I misreading the same way that you have done?

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 10/10/07, Davis Herring <herring@lanl.gov> wrote:

> I'm glad to contribute a native speaker's touch, but I'm afraid that I
> don't see what's wrong with it.  Looking at the code, it does seem that it
> invokes that hook ("fortran-unload-hook" for (unload-feature 'fortran)) if
> it exists, or else goes hunting for non-autoloaded functions defined by
> the package that are on -hook/etc. variables or on `auto-mode-alist'.  Am
> I misreading the same way that you have done?

Apparently, you're not misreading the docstring.

Each time I read this bit:

  Packages may define a hook FEATURE-unload-hook that is called
  instead of the normal heuristics for doing this.  Such a hook should
  undo all the relevant global state changes that may have been made by
  loading the package or executing functions in it.

I interpret it as if "all the relevant global state changes" refers
not just to hooks and `auto-mode-alist' (which is not mentioned in the
docstring), but to all other things that `unload-feature' does, i.e.:
killing local values of variables, f?makeunbound'ing symbols,
restoring autoloads, etc.

In other words, these sentences always make me think that either you
use `FEATURE-unload-hook' and do all the work, or you don't use it and
let `unload-feature' work its magic. Now, I *know* that's not what it
says; it's just what I read :(

In the previous discussion, David Kastrup said:

  I seem to remember that I actually had to read the code before
  getting the right idea about what FEATURE-unload-hook does.

and afterwards:

  Maybe "the normal heuristics" and "all the relevant global state
  changes" are far too much hand-waving: readers will probably
  phantasize whatever they would think useful into it.

I think these quotes summarize the problem nicely.

             Juanma

Re: unload-feature questions and thoughts

[Davis Herring]

> I interpret it as if "all the relevant global state changes" refers
> not just to hooks and `auto-mode-alist' (which is not mentioned in the
> docstring), but to all other things that `unload-feature' does, i.e.:
> killing local values of variables, f?makeunbound'ing symbols,
> restoring autoloads, etc.
>
> In other words, these sentences always make me think that either you
> use `FEATURE-unload-hook' and do all the work, or you don't use it and
> let `unload-feature' work its magic. Now, I *know* that's not what it
> says; it's just what I read :(

It seems to me that it is ambiguous, but that (as you said in your first
message) the function does the wrong thing anyway.  Perhaps, instead of
defining a return value for the hook, we should always do the heuristics
and have the FEATURE-unload-hook do only extra things, because I can't see
how the heuristics would do too much.  Moreover, the hook may modify
`unload-hook-features-list', even to nil, if it really needs to override
the heuristics.

Of course, a new doc string that describes the final behavior should be
written, but there's no point in writing two new ones.  (The doc should
probably list what the function does more explicitly, so that package
authors can see whether they need to write a hook.)

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 10/10/07, Davis Herring <herring@lanl.gov> wrote:

> Perhaps, instead of
> defining a return value for the hook, we should always do the heuristics
> and have the FEATURE-unload-hook do only extra things, because I can't see
> how the heuristics would do too much.  Moreover, the hook may modify
> `unload-hook-features-list', even to nil, if it really needs to override
> the heuristics.

Yes, that's what I've been proposing. It should have to be a new hook
for compatibility, I think.

> Of course, a new doc string that describes the final behavior should be
> written, but there's no point in writing two new ones.  (The doc should
> probably list what the function does more explicitly, so that package
> authors can see whether they need to write a hook.)

Agreed.

             Juanma

Re: unload-feature questions and thoughts

[Davis Herring]

> Yes, that's what I've been proposing. It should have to be a new hook
> for compatibility, I think.

I'm not sure the compatibility is worth the complexity.  The current
behavior is so badly documented and random (auto-mode-alist being treated
as a hook) that I would just call it a bug fix.  To check this, I went
through all the unload hooks in the sources:

delsel.el seems to think that its pre-command-hook entry will get cleared
for it, and the `remove-hook's it already does wouldn't hurt anything with
the change.

Regardless of the change, server.el needs to clear its `kill-emacs-hook'
entry (loadhist.el wouldn't, even without `server-unload-hook', because
`server-mode' is autoloaded).  It also needs to kill its C-x # binding,
suggesting that loadhist.el should perhaps sweep keymaps.  With the
change, server.el wouldn't have to do any of its extant `remove-hook's,
but they too would do no harm.

shadowfile.el needs to kill its C-x 4 s binding (again, should sweep
keymaps).  Its `remove-hook's would also be harmlessly redundant.

strokes.el wouldn't have to get rid of its `kill-emacs-query-functions'
entry itself.

whitespace.el has to have its unload function, because the functions it
adds to hooks are autoloaded.

emacs-lisp/cl.el and emacs-lisp/elp.el have meaningful unload functions too.

eshell/*.el make their unload functions customizable for some reason. 
eshell/esh-mode.el and eshell/esh-module.el are particularly odd: the
former has a default value of nil, so it just suppresses loadhist.el's
hook/a-m-a action, and the latter recursively unloads features -- it
should document that those features cannot `require' 'esh-module because
`unload-feature' would then refuse to unload it because of them even
though they would get unloaded too.

gnus/gnus-registry.el and gnus/spam-stat.el each have an interactive
unload hook that isn't necessary as an unload hook; it should be renamed
to "...-uninstall-hooks" to match its counterpart.

url/url-vars.el has a similarly needless unload hook.

So I think that the evidence is for the intuitively obvious statement that
no one knows how to use the hook.  I found no examples where the normal
heuristics would break anything, although neither did I find any examples
where the unload-hook actually broke things by suppressing the "normal
heuristics".  So I think treating it as a bug is probably the right thing
to do for simplicity: just do the heuristics regardless, and let the rare
hooks that really need to suppress them kill the variable bound for that
purpose.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: unload-feature questions and thoughts

[Richard Stallman]

    So I think that the evidence is for the intuitively obvious statement that
    no one knows how to use the hook.  I found no examples where the normal
    heuristics would break anything, although neither did I find any examples
    where the unload-hook actually broke things by suppressing the "normal
    heuristics".  So I think treating it as a bug is probably the right thing
    to do for simplicity: just do the heuristics regardless, and let the rare
    hooks that really need to suppress them kill the variable bound for that
    purpose.

Ok.

Re: unload-feature questions and thoughts

[David Kastrup]

"Davis Herring" <herring@lanl.gov> writes:

> So I think that the evidence is for the intuitively obvious
> statement that no one knows how to use the hook.  I found no
> examples where the normal heuristics would break anything, although
> neither did I find any examples where the unload-hook actually broke
> things by suppressing the "normal heuristics".  So I think treating
> it as a bug is probably the right thing to do for simplicity: just
> do the heuristics regardless, and let the rare hooks that really
> need to suppress them kill the variable bound for that purpose.

Have you checked AUCTeX's loadup/startup sequence?  It uses the unload
hooks in order to offer a way of selectively disabling autoloaded
parts.  While the "no one knows" is somewhat accurate (it was a
combination of reverse engineering and trial and error to make me
understand the implications), it is likely not true that nothing will
break by changes in that area where external packages are concerned.

And it is mostly external packages that will use those kinds of hooks.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 10/11/07, David Kastrup <dak@gnu.org> wrote:

> While the "no one knows" is somewhat accurate (it was a
> combination of reverse engineering and trial and error to make me
> understand the implications), it is likely not true that nothing will
> break by changes in that area where external packages are concerned.

I agree with Davis in believing the breakage would be small, but I'm
not necessarily in favor of introducing gratuitous incompatibilities
if they can be avoided.

So, let's just add something like the following trivial patch, which
just adds a new FEATURE-unload-function hook and leaves the old
FEATURE-unload-hook untouched (and undocumented).

The patch uses diff -b to avoid noise, and most of it is the renaming
of `unload-hook-features-list' to `unload-function-features-list'
(which not really necessary, but it'd be nice). Other than that, the
change is about four lines worth of code, less than ten if you count
an obsolescence declaration and a comment...

And of course I've not touched the docstring of `unload-feature'; that
would be for someone other than me to fix.

WDPT?

             Juanma


Index: lisp/loadhist.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/loadhist.el,v
retrieving revision 1.48
diff -u -b -r1.48 loadhist.el
--- lisp/loadhist.el	26 Jul 2007 05:26:27 -0000	1.48
+++ lisp/loadhist.el	11 Oct 2007 11:05:34 -0000
@@ -142,11 +142,13 @@
 `-hook' or `-hooks', from which `unload-feature' tries to remove
 pertinent symbols.")

-(defvar unload-hook-features-list nil
+(defvar unload-function-features-list nil
   "List of features of the package being unloaded.

-This is meant to be used by FEATURE-unload-hook hooks, see the
+This is meant to be used by FEATURE-unload-function, see the
 documentation of `unload-feature' for details.")
+(define-obsolete-variable-alias 'unload-hook-features-list
+    'unload-function-features-list "23.1")

  ;;;###autoload
  (defun unload-feature (feature &optional force)
@@ -175,19 +177,25 @@
       (when dependents
 	(error "Loaded libraries %s depend on %s"
 	       (prin1-to-string dependents) file))))
-  (let* ((unload-hook-features-list (feature-symbols feature))
-         (file (pop unload-hook-features-list))
+  (let* ((unload-function-features-list (feature-symbols feature))
+         (file (pop unload-function-features-list))
 	 ;; If non-nil, this is a symbol for which we should
  	 ;; restore a previous autoload if possible.
  	 restore-autoload
-         (unload-hook (intern-soft (concat (symbol-name feature)
-                                           "-unload-hook"))))
+	 (name (symbol-name feature))
+         (unload-hook (intern-soft (concat name "-unload-hook")))
+	 (unload-func (intern-soft (concat name "-unload-function"))))
+    ;; If FEATURE-unload-function is defined and returns non-nil,
+    ;; don't try to do anything more; otherwise proceed normally.
+    (unless (and (bound-and-true-p unload-func)
+		 (funcall unload-func))
     ;; Try to avoid losing badly when hooks installed in critical
     ;; places go away.  (Some packages install things on
     ;; `kill-buffer-hook', `activate-menubar-hook' and the like.)
+      (if unload-hook
     ;; First off, provide a clean way for package FOO to arrange
     ;; this by adding hooks on the variable `FOO-unload-hook'.
-    (if unload-hook
+	  ;; This is obsolete; FEATURE-unload-function should be used now.
         (run-hooks unload-hook)
       ;; Otherwise, do our best.  Look through the obarray for symbols
       ;; which seem to be hook variables or special hook functions and
@@ -202,22 +210,22 @@
  		    (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-hook-features-list)
+	     (dolist (y unload-function-features-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-hook-features-list)
+	(dolist (y unload-function-features-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)))))
     (when (fboundp 'elp-restore-function) ; remove ELP stuff first
-      (dolist (elt unload-hook-features-list)
+	(dolist (elt unload-function-features-list)
  	(when (symbolp elt)
  	  (elp-restore-function elt))))

-    (dolist (x unload-hook-features-list)
+      (dolist (x unload-function-features-list)
       (if (consp x)
 	  (case (car x)
 	   ;; Remove any feature names that this file provided.
@@ -250,7 +258,7 @@
 	(unless (local-variable-if-set-p x)
  	  (makunbound x))))
     ;; Delete the load-history element for this file.
-    (setq load-history (delq (assoc file load-history) load-history)))
+      (setq load-history (delq (assoc file load-history) load-history))))
   ;; Don't return load-history, it is not useful.
   nil)

Re: unload-feature questions and thoughts

[Richard Stallman]

    So, let's just add something like the following trivial patch, which
    just adds a new FEATURE-unload-function hook and leaves the old
    FEATURE-unload-hook untouched (and undocumented).

If we are not going to change the meaning of FEATURE-unload-hook, then
rather than create a second similar feature, I would rather move the
normal handling into a function that the hooks can call.

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 10/12/07, Richard Stallman <rms@gnu.org> wrote:

> If we are not going to change the meaning of FEATURE-unload-hook, then
> rather than create a second similar feature, I would rather move the
> normal handling into a function that the hooks can call.

IMHO this is a hack. `unload-feature' does quite a bit of things (and
could conceivably do more, as Davis suggested: remove items from
keymaps, for example). There's no need for a hook that only
substitutes for part of that processing. What if in my package the
default hook processing of `unload-feature' is fine, but I want to do
my own autoload restore?

The right thing to do is let a hook run *before* all that happens, so
it can influence what is done afterwards (by saying that it doesn't
want more processing, or by modifying `unload-hook-features-list').

Let's get rid of FEATURE-unload-hook. It is used only when unloading
packages, so in most cases the incompatibility won't cause big trouble
(unless unloading packages is more common that I think).

             Juanma

Re: unload-feature questions and thoughts

[Richard Stallman]

    > If we are not going to change the meaning of FEATURE-unload-hook, then
    > rather than create a second similar feature, I would rather move the
    > normal handling into a function that the hooks can call.

    IMHO this is a hack. `unload-feature' does quite a bit of things (and
    could conceivably do more, as Davis suggested: remove items from
    keymaps, for example). There's no need for a hook that only
    substitutes for part of that processing.

I don't know why you say it "only substitutes for part of that
processing".  Maybe it is a misunderstanding.

    Let's get rid of FEATURE-unload-hook. It is used only when unloading
    packages, so in most cases the incompatibility won't cause big trouble
    (unless unloading packages is more common that I think).

That is gratuitous breakage.  No thanks.

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 10/13/07, Richard Stallman <rms@gnu.org> wrote:

> I don't know why you say it "only substitutes for part of that
> processing".  Maybe it is a misunderstanding.

unload-feature does this:

 - Remove functions from hooks (those in
`unload-feature-special-hook') and from `auto-mode-alist'.
 - Remove ELP stuff
 - Remove feature names
 - Restores autoloads
 - Kills local variables
 - (f)?makunbound's symbols
 - Kills timers
 - Deletes the load-history element for the file

A package-defined FEATURE-unload-hook is run as an alternative to just
*the first* of these items; the rest are done irregardless of the
existence of FEATURE-unload-hook (though it can affect them in part by
modifying `unload-hook-features-list').

> That is gratuitous breakage.  No thanks.

Well, you don't want to get rid of FEATURE-unload-hook, and you don't
want to add FEATURE-unload-function and have two similar mechanisms.

What is the alternative: retain FEATURE-unload-hook and make it do
something else? Because, if I'm understanding you right, what you
propose is turning the above list of unloading effects into one or
several functions that can be called from FEATURE-unload-hook, and
that is IMHO a hack.

             Juanma

Re: unload-feature questions and thoughts

[Richard Stallman]

    unload-feature does this:

     - Remove functions from hooks (those in
    `unload-feature-special-hook') and from `auto-mode-alist'.
     - Remove ELP stuff
     - Remove feature names
     - Restores autoloads
     - Kills local variables
     - (f)?makunbound's symbols
     - Kills timers
     - Deletes the load-history element for the file

    A package-defined FEATURE-unload-hook is run as an alternative to just
    *the first* of these items;

Now I see what you mean.  I am not sure why or whether it was
good to set up this feature as it is.  However, that doesn't
necessarily mean we should just get rid of it or break it.

If we can see that the files which use it use it are in general wrong
and are broken, then we can conclude it doesn't matter if we change
this incompatibly.  If we can see that the files which use it would
still work despite an incompatible change, then that change is ok.
Otherwise, we should make a compatible change.

Davis Herring looked at the various FEATURE-unload-hooks and reported
on their problems.  But the report doesn't directly answer
the two crucial questions above.

Davis, can you look at your previous results and answer those
questions?

People have suggested two ways to make a compatible change.
One is to add FEATURE-unload-function in parallel.
The other is to move the standard code to delete from hooks
into a subroutine which FEATURE-unload-hook can call.

I prefer the latter way.

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 10/14/07, Richard Stallman <rms@gnu.org> wrote:

> People have suggested two ways to make a compatible change.
> One is to add FEATURE-unload-function in parallel.
> The other is to move the standard code to delete from hooks
> into a subroutine which FEATURE-unload-hook can call.
>
> I prefer the latter way.

But these are not equivalent. FEATURE-unload-function would allow
packages with uncommon unload strategies to do all unloading by
themselves (by passing a flag back to unload-feature not to do any
more unloading), but still use the unload-feature machinery to check
feature dependencies, for example.

             Juanma

Re: unload-feature questions and thoughts

[Richard Stallman]

    But these are not equivalent. FEATURE-unload-function would allow
    packages with uncommon unload strategies to do all unloading by
    themselves (by passing a flag back to unload-feature not to do any
    more unloading), but still use the unload-feature machinery to check
    feature dependencies, for example.

Oh, now I see.  So after FEATURE-unload-function, unload-feature
would do either ALL the rest of the standard processing, or none?

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 10/15/07, Richard Stallman <rms@gnu.org> wrote:

> Oh, now I see.  So after FEATURE-unload-function, unload-feature
> would do either ALL the rest of the standard processing, or none?

That's the idea, though for finer-grained control
FEATURE-unload-function can always modify `unload-hook-features-list'
before returning the "continue the standard processing" flag back to
`unload-feature'.

             Juanma

Re: unload-feature questions and thoughts

[Richard Stallman]

    That's the idea, though for finer-grained control
    FEATURE-unload-function can always modify `unload-hook-features-list'
    before returning the "continue the standard processing" flag back to
    `unload-feature'.

How would that be useful?  What problem could it solve?

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 10/16/07, Richard Stallman <rms@gnu.org> wrote:

>     That's the idea, though for finer-grained control
>     FEATURE-unload-function can always modify `unload-hook-features-list'
>     before returning the "continue the standard processing" flag back to
>     `unload-feature'.
>
> How would that be useful?  What problem could it solve?

Well, that's how `unload-hook-features-list' is used right now,
according to docstrings.

From `unload-hook-features-list':

  This is meant to be used by FEATURE-unload-hook hooks, see the
  documentation of `unload-feature' for details.

From `unload-feature':

  Such a hook should undo all the relevant global state changes that may
  have been made by loading the package or executing functions in it.
  It has access to the package's feature list (before anything is unbound)
  in the variable `unload-hook-features-list' and could remove features
  from it in the event that the package has done something strange,
  such as redefining an Emacs function.


             Juanma

Re: unload-feature questions and thoughts

[Richard Stallman]

I guess your idea is a good one.  Would you like to implement it?

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 10/17/07, Richard Stallman <rms@gnu.org> wrote:

> I guess your idea is a good one.  Would you like to implement it?

The implementation is trivial (see the attached patch), other than
updating the docstring of `unload-feature', which I'm unable to do.

             Juanma


2007-10-17  Juanma Barranquero  <lekktu@gmail.com>

	* loadhist.el (unload-function-features-list):
	Rename from `unload-hook-features-list'.
	(unload-hook-features-list): Add as obsolete alias.
	(unload-feature): Use `unload-function-features-list'
	and new FEATURE-unload-function.

Index: lisp/loadhist.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/loadhist.el,v
retrieving revision 1.50
diff -c -b -r1.50 loadhist.el
*** lisp/loadhist.el	17 Oct 2007 23:03:55 -0000	1.50
--- lisp/loadhist.el	17 Oct 2007 23:16:28 -0000
***************
*** 137,147 ****
  `-hook' or `-hooks', from which `unload-feature' tries to remove
  pertinent symbols.")

! (defvar unload-hook-features-list nil
    "List of features of the package being unloaded.

! This is meant to be used by FEATURE-unload-hook hooks, see the
  documentation of `unload-feature' for details.")

  ;;;###autoload
  (defun unload-feature (feature &optional force)
--- 137,149 ----
  `-hook' or `-hooks', from which `unload-feature' tries to remove
  pertinent symbols.")

! (defvar unload-function-features-list nil
    "List of features of the package being unloaded.

! This is meant to be used by FEATURE-unload-function, see the
  documentation of `unload-feature' for details.")
+ (define-obsolete-variable-alias 'unload-hook-features-list
+     'unload-function-features-list "23.1")

  ;;;###autoload
  (defun unload-feature (feature &optional force)
***************
*** 172,190 ****
        (when dependents
  	(error "Loaded libraries %s depend on %s"
  	       (prin1-to-string dependents) file))))
!   (let* ((unload-hook-features-list (feature-symbols feature))
!          (file (pop unload-hook-features-list))
  	 ;; If non-nil, this is a symbol for which we should
  	 ;; restore a previous autoload if possible.
  	 restore-autoload
!          (unload-hook (intern-soft (concat (symbol-name feature)
!                                            "-unload-hook"))))
      ;; Try to avoid losing badly when hooks installed in critical
      ;; places go away.  (Some packages install things on
      ;; `kill-buffer-hook', `activate-menubar-hook' and the like.)
      ;; First off, provide a clean way for package FOO to arrange
      ;; this by adding hooks on the variable `FOO-unload-hook'.
!     (if unload-hook
          (run-hooks unload-hook)
        ;; Otherwise, do our best.  Look through the obarray for symbols
        ;; which seem to be hook variables or special hook functions and
--- 174,198 ----
        (when dependents
  	(error "Loaded libraries %s depend on %s"
  	       (prin1-to-string dependents) file))))
!   (let* ((unload-function-features-list (feature-symbols feature))
!          (file (pop unload-function-features-list))
  	 ;; If non-nil, this is a symbol for which we should
  	 ;; restore a previous autoload if possible.
  	 restore-autoload
! 	 (name (symbol-name feature))
!          (unload-hook (intern-soft (concat name "-unload-hook")))
! 	 (unload-func (intern-soft (concat name "-unload-function"))))
!     ;; If FEATURE-unload-function is defined and returns non-nil,
!     ;; don't try to do anything more; otherwise proceed normally.
!     (unless (and (bound-and-true-p unload-func)
! 		 (funcall unload-func))
        ;; Try to avoid losing badly when hooks installed in critical
        ;; places go away.  (Some packages install things on
        ;; `kill-buffer-hook', `activate-menubar-hook' and the like.)
+       (if unload-hook
  	  ;; First off, provide a clean way for package FOO to arrange
  	  ;; this by adding hooks on the variable `FOO-unload-hook'.
! 	  ;; This is obsolete; FEATURE-unload-function should be used now.
  	  (run-hooks unload-hook)
  	;; Otherwise, do our best.  Look through the obarray for symbols
  	;; which seem to be hook variables or special hook functions and
***************
*** 199,220 ****
  		    (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-hook-features-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-hook-features-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)))))
      (when (fboundp 'elp-restore-function) ; remove ELP stuff first
!       (dolist (elt unload-hook-features-list)
  	(when (symbolp elt)
  	  (elp-restore-function elt))))

!     (dolist (x unload-hook-features-list)
        (if (consp x)
  	  (case (car x)
  	   ;; Remove any feature names that this file provided.
--- 207,228 ----
  		      (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-features-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-features-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)))))
        (when (fboundp 'elp-restore-function) ; remove ELP stuff first
! 	(dolist (elt unload-function-features-list)
  	  (when (symbolp elt)
  	    (elp-restore-function elt))))

!       (dolist (x unload-function-features-list)
  	(if (consp x)
  	    (case (car x)
  	      ;; Remove any feature names that this file provided.
***************
*** 247,253 ****
  	(unless (local-variable-if-set-p x)
  	  (makunbound x))))
      ;; Delete the load-history element for this file.
!     (setq load-history (delq (assoc file load-history) load-history)))
    ;; Don't return load-history, it is not useful.
    nil)

--- 255,261 ----
  	  (unless (local-variable-if-set-p x)
  	    (makunbound x))))
        ;; Delete the load-history element for this file.
!       (setq load-history (delq (assoc file load-history) load-history))))
    ;; Don't return load-history, it is not useful.
    nil)

Re: unload-feature questions and thoughts

[Davis Herring]

> Davis, can you look at your previous results and answer those
> questions?

I can do this at some point, but I am quite busy at the moment, so it may
easily be a week from now.  I'll try to review my notes faster than that,
but I want the possibility that it is slow to be known.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: unload-feature questions and thoughts

[Richard Stallman]

A week from now is better than never.  Please respond to this message
with the answers.

Re: unload-feature questions and thoughts

[Davis Herring]

>> So I think that the evidence is for the intuitively obvious
>> statement that no one knows how to use the hook.  I found no
>> examples where the normal heuristics would break anything, although
>> neither did I find any examples where the unload-hook actually broke
>> things by suppressing the "normal heuristics".  So I think treating
>> it as a bug is probably the right thing to do for simplicity: just
>> do the heuristics regardless, and let the rare hooks that really
>> need to suppress them kill the variable bound for that purpose.
>
> Have you checked AUCTeX's loadup/startup sequence?  It uses the unload
> hooks in order to offer a way of selectively disabling autoloaded
> parts.  While the "no one knows" is somewhat accurate (it was a
> combination of reverse engineering and trial and error to make me
> understand the implications), it is likely not true that nothing will
> break by changes in that area where external packages are concerned.

First, a correction to my message and in particular the summary you
quoted: delsel.el did break things with its unload-hook, since (at least
in the current buffer) it would otherwise have had its pre-command-hook
entry removed, but as it stands that will just error out immediately after
the unload.  (From this I note that buffers should be swept for local hook
values as well as local variable values.)

Second, I just looked at the one "unload-hook" I found (in
tex-site.el.in), and it seems to just be manipulating `after-load-alist'
(though it forgets to setq it at the end) and `load-path'.  So it too
would benefit from having the normal heuristics applied (in case the user
put random AUCTeX functions on hooks).  Am I missing something?

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: unload-feature questions and thoughts

[Richard Stallman]

    Have you checked AUCTeX's loadup/startup sequence?  It uses the unload
    hooks in order to offer a way of selectively disabling autoloaded
    parts.

Could you tell us how AUCTeX's hooks would be affected by the various
proposed changes?

Re: unload-feature questions and thoughts

[David Kastrup]

Richard Stallman <rms@gnu.org> writes:

>     Have you checked AUCTeX's loadup/startup sequence?  It uses the
>     unload hooks in order to offer a way of selectively disabling
>     autoloaded parts.
>
> Could you tell us how AUCTeX's hooks would be affected by the
> various proposed changes?

Actually, I don't really have time in the next few days to do the
proper research.

One somewhat surprising thing I remembered is that it was necessary to
call an autoloaded (dummy in this case) function instead of
straightforwardly using require.  The latter did not register the
necessary information for unloading the feature again (unloading the
feature needs to restore autoloads for the default TeX modes).

To wit, we have
;;; auctex.el
;;
;; This can be used for starting up AUCTeX.  The following somewhat
;; strange trick causes tex-site.el to be loaded in a way that can be
;; safely undone using (unload-feature 'tex-site).
;;
(autoload 'TeX-load-hack
  @lisptexsite@)
(TeX-load-hack)

Where @lisptexsite@ resolves to the full path of the actual startup
file tex-site.el.

This file contains the following relevant stuff:

(defalias 'TeX-load-hack 'ignore)

(add-hook 'tex-site-unload-hook
	  (lambda ()
	    (let ((list after-load-alist))
	      (while list
		;; Adapted copy of the definition of `assq-delete-all'
		;; from Emacs 21 as substitute for
		;; `(assq-delete-all'TeX-modes-set (car list))' which
		;; fails on non-list elements in Emacs 21.
		(let* ((alist (car list))
		       (tail alist)
		       (key 'TeX-modes-set))
		  (while tail
		    (if (and (consp (car tail))
			     (eq (car (car tail)) key))
			(setq alist (delq (car tail) alist)))
		    (setq tail (cdr tail))))
		(setq list (cdr list))))
	    (setq load-path (delq TeX-lisp-directory load-path))))

(defun TeX-modes-set (var value &optional update)
  "Set VAR (which should be `TeX-modes') to VALUE.

This places either the standard or the AUCTeX versions of
functions into the respective function cell of the mode.
If UPDATE is set, a previously saved value for
the non-AUCTeX function gets overwritten with the current
definition."
  (custom-set-default var value)
  (let ((list TeX-mode-alist) elt)
    (while list
      (setq elt (car (pop list)))
      (when (or update (null (get elt 'tex-saved)))
	(when (fboundp elt)
	  (put elt 'tex-saved (symbol-function elt))))
      (defalias elt
	(if (memq elt value)
	    (intern (concat "TeX-" (symbol-name elt)))
	  (get elt 'tex-saved))))))

(defcustom TeX-modes
  (mapcar 'car TeX-mode-alist)
  "List of modes provided by AUCTeX.

This variable can't be set normally; use customize for that, or
set it with `TeX-modes-set'."
  :type (cons 'set
	      (mapcar (lambda(x) (list 'const (car x))) TeX-mode-alist))
  :set 'TeX-modes-set
  :group 'AUCTeX
  :initialize (lambda (var value)
		(custom-initialize-reset var value)
		(let ((list TeX-mode-alist))
		  (while list
		    (eval-after-load (cdar list)
		      `(TeX-modes-set ',var ,var t))
		    (setq list (cdr list))))))


This is all rather messy and I don't remember all too well what all of
this does for which exact reason, but the key point is that the
variable TeX-modes can be set to overwrite a selected subset of mode
definition autoloads with autoloads for the respective AUCTeX version
of the mode, and (unload-feature 'tex-site) will pretty much undo
(load-file "auctex.el") as long as none of the autoloads overwritten
in tex-site.el have yet been triggered.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: unload-feature questions and thoughts

[Richard Stallman]

    One somewhat surprising thing I remembered is that it was necessary to
    call an autoloaded (dummy in this case) function instead of
    straightforwardly using require.  The latter did not register the
    necessary information for unloading the feature again (unloading the
    feature needs to restore autoloads for the default TeX modes).

I think we recently fixed that:

2007-10-14  Juanma Barranquero  <lekktu@gmail.com>

	* eval.c (do_autoload): Don't save autoloads.

	* data.c (Ffset): Save autoload of the function being set.


Anyway, we've installed the new feature for unload hooks for Emacs 22;
could you please DTRT (whatever that is) in AUCTeX?

Re: unload-feature questions and thoughts

[Richard Stallman]

I will rewrite the doc string.  Thanks.

    All in all, I think it'd be useful to have a FEATURE-unload-function
    which would return t or nil to signal whether it has done everything,
    or would like for unload-feature to continue with its normal
    processing.

The idea makes sense, but it would be an incompatible change.  A
compatible change with a similar effect is to move the usual
processing into a separate function which the hook can call.
Want to do that?

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 10/10/07, Richard Stallman <rms@gnu.org> wrote:

> I will rewrite the doc string.

Thanks.

> The idea makes sense, but it would be an incompatible change.  A
> compatible change with a similar effect is to move the usual
> processing into a separate function which the hook can call.
> Want to do that?

I think Davis Herring has made a compelling argument that the current
behavior is confusing and the new one could be implemented with minor
incompatibilities, if any.

             Juanma

Re: unload-feature questions and thoughts

[Richard Stallman]

     1.- Currently, `unload-feature' (from loadhist.el) does not expect
    entries on `load-history' of the form (autoload . SYMBOL) or (defface
    . SYMBOL). Unloading a package which defines either one produces
    messages like

      Unexpected element (defface . myface) in load-history

    which should be reserved, I think, to really unexpected items in the
    load-history, not perfectly usual ones like those above.

Since the presence of such entries is normal, `unload-feature' ought
to handle such entries in a useful way.

Would someone like to implement this?

    AFAICS, there's no way to delete an existing face, so ignoring the
    item is the best option (or, alternatively, giving a more significant
    warning, like "Face MYFACE can not be unloaded").

Of these two, it would definitely be better to do nothing.
Signaling an error in circumstances that are normal is perverse.

It should not be hard to add a way to delete a face, but let's leave
that for later.

						      Autoload entries,
    OTOH, can be unloaded like a function; so I propose to commit the
    attached patch.

Your patch to handle an `autoload' entry like an `defun' entry seems
correct.

Meanwhile, I think another change is needed to handle `(t . SYMBOL)'
elements correctly.  See below.  Does anyone else see a problem in it?

     2.- `unload-feature' returns `load-history', but this is not
    documented, and a PITA when you're using `unload-feature' in a context
    that evaluates its result, like IELM. Should we document what it does
    now, or (preferred) just force it to return nil?

It is ok to make it return nil.



*** loadhist.el	21 Jan 2007 01:36:08 -0500	1.43
--- loadhist.el	05 Feb 2007 13:30:24 -0500	
***************
*** 173,178 ****
--- 173,181 ----
  	       (prin1-to-string dependents) file))))
    (let* ((unload-hook-features-list (feature-symbols feature))
           (file (pop unload-hook-features-list))
+ 	 ;; If non-nil, this is a symbol for which we should
+ 	 ;; restore a previous autoload if possible.
+ 	 restore-autoload
           (unload-hook (intern-soft (concat (symbol-name feature)
                                             "-unload-hook"))))
      ;; Try to avoid losing badly when hooks installed in critical
***************
*** 209,214 ****
--- 212,218 ----
        (dolist (elt unload-hook-features-list)
  	(when (symbolp elt)
  	  (elp-restore-function elt))))
+ 
      (dolist (x unload-hook-features-list)
        (if (consp x)
  	  (case (car x)
***************
*** 221,230 ****
  		(when (fboundp 'ad-unadvise)
  		  (ad-unadvise fun))
  		(let ((aload (get fun 'autoload)))
! 		  (if aload
                        (fset fun (cons 'autoload aload))
                      (fmakunbound fun))))))
!            ((t require) nil)
  	   (t (message "Unexpected element %s in load-history" x)))
  	;; Kill local values as much as possible.
  	(dolist (buf (buffer-list))
--- 225,238 ----
  		(when (fboundp 'ad-unadvise)
  		  (ad-unadvise fun))
  		(let ((aload (get fun 'autoload)))
! 		  (if (and aload (eq fun restore-autoload))
                        (fset fun (cons 'autoload aload))
                      (fmakunbound fun))))))
! 	   ;; (t . SYMBOL) comes before (defun . SYMBOL)
! 	   ;; and says we should restore SYMBOL's autoload
! 	   ;; when we undefine it.
! 	   (t (setq restore-autoload (cdr x)))
!            (require nil)
  	   (t (message "Unexpected element %s in load-history" x)))
  	;; Kill local values as much as possible.
  	(dolist (buf (buffer-list))

Re: unload-feature questions and thoughts

[Juanma Barranquero]

On 2/5/07, Richard Stallman <rms@gnu.org> wrote:

> Since the presence of such entries is normal, `unload-feature' ought
> to handle such entries in a useful way.
>
> Would someone like to implement this?

That's what my patch does: the useful thing for (autoload . SYMBOL) is
treat it like a defun, and the useful thing for (defface . SYMBOL) is
ignore it.

> Meanwhile, I think another change is needed to handle `(t . SYMBOL)'
> elements correctly.  See below.  Does anyone else see a problem in it?

It seems correct, except that

> !          (t (setq restore-autoload (cdr x)))

should be

> !          ((t) (setq restore-autoload (cdr x)))

because a t alone is equivalent to `otherwise'.

                    /L/e/k/t/u

Re: unload-feature questions and thoughts

[Richard Stallman]

    That's what my patch does: the useful thing for (autoload . SYMBOL) is
    treat it like a defun, and the useful thing for (defface . SYMBOL) is
    ignore it.

Please install your patch.

    It seems correct, except that

    > !          (t (setq restore-autoload (cdr x)))

Thanks for correcting it.