add-hook

add-hook

[Richard Stallman]

Today I took another look at the code in add-hook and this time I
could clearly see the problem that I originally fixed.  This code

    ;; Detect the case where make-local-variable was used on a hook
    ;; and do what we used to do.
    (unless (and (consp (symbol-value hook)) (memq t (symbol-value hook)))
      (setq local t)))

claims to detect the case where the hook variable was made local in
the wrong way, but it doesn't really do that.  It will set `local' to
t in cases where the hook is not actually local.  In fact, in the
simplest case, where the hook variable has no local binding, and no
local hooks have ever been put on it, this code will still set `local'
to t.

That is very confusing, and contradicts what the comment says.
This needs to be cleaned up somehow.

Re: add-hook

[Miles Bader]

Richard Stallman <rms@gnu.org> writes:
>     ;; Detect the case where make-local-variable was used on a hook
>     ;; and do what we used to do.
>     (unless (and (consp (symbol-value hook)) (memq t (symbol-value hook)))
>       (setq local t)))
> 
> That is very confusing, and contradicts what the comment says.
> This needs to be cleaned up somehow.

Call me insane, but wouldn't `local-variable-p' be a nice way to detect
this case...?

-Miles
-- 
o The existentialist, not having a pillow, goes everywhere with the book by
  Sullivan, _I am going to spit on your graves_.

Re: add-hook

[Stefan Monnier]

> Richard Stallman <rms@gnu.org> writes:
>> ;; Detect the case where make-local-variable was used on a hook
>> ;; and do what we used to do.
>> (unless (and (consp (symbol-value hook)) (memq t (symbol-value hook)))
>> (setq local t)))
>> 
>> That is very confusing, and contradicts what the comment says.
>> This needs to be cleaned up somehow.

> Call me insane, but wouldn't `local-variable-p' be a nice way to detect
> this case...?

Well, that's what RMS thought and so he added it and it breaks things:
Setting `local' to t ensures that the var will be modified with `set'
whereas a setting of nil implies that the hook will be modified with
`set-default', so the safe setting is t.  The only case where we
want to use the unsafe setting of nil is when we indeed want to change
the global part of the hook and there is both a local and a global part
(i.e when local == nil and when there's a t in the hook's content).

You can add an unnecessary test for local-variable-if-set-p, or you can
change the comment to make things more clear, but you can't use
local-variable-p: we tried that already.


        Stefan

Re: add-hook

[Miles Bader]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
> or you can change the comment to make things more clear

How about:

;; Setting `local' to t ensures that the var will be modified with `set'
;; whereas a setting of nil implies that the hook will be modified with
;; `set-default', so the safe setting is t.  The only case where we
;; want to use the unsafe setting of nil is when we indeed want to change
;; the global part of the hook and there is both a local and a global part
;; (i.e when local == nil and when there's a t in the hook's content).

:-)

-Miles
-- 
Next to fried food, the South has suffered most from oratory.
  			-- Walter Hines Page

Re: add-hook

[Richard Stallman]

    ;; Setting `local' to t ensures that the var will be modified with `set'
    ;; whereas a setting of nil implies that the hook will be modified with
    ;; `set-default', so the safe setting is t.  The only case where we
    ;; want to use the unsafe setting of nil is when we indeed want to change
    ;; the global part of the hook and there is both a local and a global part
    ;; (i.e when local == nil and when there's a t in the hook's content).

That comment may be correct, but it is still hard to understand.
Also, the variable ought to be renamed.  t does not mean the hook
is local or that anything is local.

Re: add-hook

[Richard Stallman]

    > That is very confusing, and contradicts what the comment says.
    > This needs to be cleaned up somehow.

    Call me insane, but wouldn't `local-variable-p' be a nice way to detect
    this case...?

That is the change I made.  But Stefan said it made the code
incomprehensible to him.  It also did apparently cause BBDB to
malfunction.