[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] (Updated) Run hook when variable is set
From: |
Stefan Monnier |
Subject: |
Re: [PATCH] (Updated) Run hook when variable is set |
Date: |
Thu, 12 Feb 2015 14:58:13 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.0.50 (gnu/linux) |
>> No, the idea was rather to do:
>>
>> (defun set-internal-1 (args)
>> (block nil ; Because Elisp isn't CL
>> (if (constant-or-hooked-p)
>> (if (constant-p)
>> (if (forbidden-p)
>> (error "setting constant")
>> (return))
>> (funcall symbol-watch-function ..args..))
>> (do-some-stuff)
>> (and-many-more-lines-of-stuff)
>> (set-some-variable))))
> Then the many more lines of stuff have to be copied into
> symbol-watch-function. Those lines can't just be skipped;
We don't really need to copy those lines, instead symbol-watch-function
should somehow (in the "normal" case) call us back, and in way that
doesn't trigger the (constant-or-hooked-p) check.
Of course, for that "...args..." should have enough info that we can
indeed "call it back" properly.
> By default, hooking a symbol must not change the behavior of Emacs,
> except for slowing it down. A hook handler can optionally change the
> behavior, but just hooking the symbol must cause no change if the
> handler doesn't.
Agreed.
> Updated patches attached. The first applies to 24.4.
We don't want to install this in emacs-24, so only the trunk (aka
"master") code is important in this respect.
[ BTW, even for 30KB patches, I prefer them uncompressed, because
I typically read them directly in Gnus. ]
> + /* When masked with SYMBOL_CONSTANT_MASK, non-zero means symbol is
> + constant, i.e. changing its value should signal an error. If the
> + value is 3, then the var can be changed, but only by `defconst'.
> + When masked with SYMBOL_HOOKED_MASK, non-zero means setting
> + symbol will run varhook. These two fields are combined into one
> + in order to optimize the fast path of unhooked non-constants by
> + having only one conditional branch for that case. */
We don't need this special value 3 for defconst, not only because the
code doesn't use it, but also since if we want that unimplemented
functionality, we can now implement it using your new
symbol-setter system.
> +typedef enum
> + {
> + Dyn_Unbind = -1,
> + Dyn_Current = 0,
> + Dyn_Bind = 1,
> + Dyn_Skip = 2,
> + Dyn_Global = 3
> + } Dyn_Bind_Direction;
In which sense is this a "direction"?
> +/* Like set_internal but with direction argument to indicate whether this
> + function call is due to a binding (1), an unbinding (-1), or neither (0).
> + As special cases, a value of 2 is a flag to disable run_varhook so that
> + varhooks aren't run during backtraces, and a value of 3 is a flag
> + indicating that this function call is due to set_default, which allows
> + run_varhook to distinguish beween the global and the dyn-local binding.
> */
Please use the Dyn_* names rather than the numerical constants in
the comment.
> + Return the result of symbol-setter-function. The variable will be set
> + (by code that calls run_varhook) to that result, overriding the value to
> + which the setter is attempting to set the variable. */
That's a good idea, to circumvent the question of how to not-trigger the
hooked-p check recursively when the hook function calls the setter (tho
the question partly remains, in case the hook function *accidentally*
sets one of the hooked variables).
It does mean that the hooks can't redirect the assignment elsewhere, but
maybe it's a good thing anyway.
> + bool shadowed;
> + if (buf_local)
> + shadowed = let_shadows_buffer_binding_p (sym);
> + else shadowed = let_shadows_global_binding_p (symbol);
Aka
bool shadowed = (buf_local ? let_shadows_buffer_binding_p (sym)
: let_shadows_global_binding_p (symbol));
> + if (shadowed) env = Qdyn_local;
> + else if (buf_local) env = Qbuf_local;
> + else env = Qglobal;
Why does the hook need to know about those different cases?
> + start: /* Just to avoid reindentation in the varhook patch */
"diff -bw" works just as well for me.
> + (XFWDTYPE (innercontents))==Lisp_Fwd_Buffer_Obj,
^^
We like to put spaces around infix operators.
> +DEFUN ("symbol-setter-function", Fsymbol_setter_function,
> Ssymbol_setter_function, 4, 4, 0,
Hmm, no symbol-setter-function should be a variable (holding
a function), modified via add-function/remove-function.
Also the docstring should not recommend :override (which should be
a rather rare case, the more useful cases are probably :before
and :around), and in general it should just say how the function is
called and what it should return, without giving a primer about how to
modify such *-function variables, since we don't want to do that for
every *-function variable.
OTOH the docstring should document the different possible values of ENV
and their respective meaning.
Stefan
- Re: [PATCH] Run hook when variable is set, (continued)
- Re: [PATCH] Run hook when variable is set, Stefan Monnier, 2015/02/02
- Re: [PATCH] Run hook when variable is set, Kelly Dean, 2015/02/03
- Re: [PATCH] Run hook when variable is set, Stefan Monnier, 2015/02/03
- [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/04
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/05
- Re: [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/06
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/06
- Re: [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/07
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/07
- [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/08
- Re: [PATCH] (Updated) Run hook when variable is set,
Stefan Monnier <=
- [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/13
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/13
- Re: [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/14
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/15
- [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/16
- Re: [PATCH] (Updated) Run hook when variable is set, Richard Stallman, 2015/02/17
- The purpose of makunbound (Was: Run hook when variable is set), Kelly Dean, 2015/02/17
- Re: The purpose of makunbound, Stefan Monnier, 2015/02/18
- Re: The purpose of makunbound, Kelly Dean, 2015/02/18
- Re: The purpose of makunbound, Stefan Monnier, 2015/02/18