[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: |
Sun, 22 Feb 2015 23:19:00 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.0.50 (gnu/linux) |
>> I'm sorry, but I consider this a form of lying. It makes it sound like
>> "the new var-hook functionality actually speeds things up", even though
>> it's not this new functionality but some unrelated (tho bundled) change
>> which does it (and which could be applied independently).
> I didn't say the varhook functionality speeds things up. I explicitly said
> it has performance costs, and I found optimization opportunities to _pay
> for_ those costs.
I know. I said "makes it sound like".
> I was completely honest about what I did,
You were, indeed.
>> The issue is not patch size per se, but just keeping the patch focused
>> on its core purpose.
> The constant-hooked combination is an optimization,
I don't consider it an optimization. I consider the functionality to be
an extension of the "constant" bit into a more nuanced 3-valued field.
> The patch could be applied to trunk without that optimization,
No because it's fundamentally wrong to have `constant' in one field and
`hooked' in another when `hooked' can only be used when constant==0.
> Fine, but adding the capability of converting setq, etc into makunbound has
> the cost of breaking correctness (because hooking a symbol would change the
> behavior of (setq foo void-sentinel)), not just the cost of enabling user
> errors.
I think that would be OK because (setq foo void-sentinel) can be
considered as erroneous code.
This said, I don't care very much about allowing/disallowing a setq to
turn into a makunbound. I think it's not worth spending any code on this,
so I similarly shouldn't spend much time arguing about it.
> Magic constants added back in to the source code, just to avoid
> touching a few lines of code in the patch.
Actually, this is a mistake. Using 1 was OK back when the code was
written (it should be `true' instead nowadays), but with the new
definition of the field, it should be SYM_CONST.
> This version of the patch slows down Emacs.
How much, where, and why? Clearly, there is an added cost in the
unbind_to case since we now have to check sym->constant, but otherwise,
where the extra cost come from? Is it just the extra `env' argument to
set_internal?
> IIUC, I've made all the changes you requested.
Indeed, thank you. See included the remaining nitpicks below.
The only significant comments below is the one about the ENV arg, since
I think we really should be able to distinguish all buffer-local changes
from the non-buffer-local ones (I think this is more important than
distinguishing `setq' to the toplevel value vs `setq' within a `let',
which we could even distinguish without any special ENV value by
exporting let_shadows_buffer_binding_p and/or
let_shadows_global_binding_p to Elisp).
Stefan
> +SYMBOL is the symbol being set. ENV is the environment is which it's being
> +set.
Actually, ENV is not an environment (it's just a symbol which describes
in which environment the modification is made).
> +The possible values of ENV are these symbols, with these meanings:
> +global: The global environment.
So, this is for a `setq-default' or a `setq' when there's no
buffer-local value, and no active let-binding.
> +buf-local: The setter's buffer-local environment. ENV is this value if the
> +setter sets the buffer-local variable.
So, IIUC this is for a `setq' when the variable has been made buffer
local in this buffer.
And IIUC this is for the case where there's no active let-binding for
this variable, right?
Does it distinguish between let-bindings for the buffer-local part of
this variable, and let-bindings for other parts of the variable (e.g. in
other buffers, or let-binding for the global part of the var)?
> +dyn-local: The innermost dynamic environment in which SYMBOL is bound. ENV
> +is this value if the setter sets a dynamic local variable.
IIUC this is for a `setq' to a variable that has an active let-binding.
Does it distinguish between the case where the let-binding is
buffer-local and where it isn't?
> +dyn-bind: A new dynamic environment. ENV is this value if the setter creates
> +a new dynamic environment, such as by using `let'.
This is for a `let'. But IIUC we don't get to know if this let is
buffer-local or global, right?
> +to NEWVAL, return NEWVAL. To block the attempt, and leave the variable
^^^
BTW, we use the `sentence-end-double-space' convention in our comments
and docstrings.
> +is the value of symbol-hook-void-value but NEWVAL is not, you can
^^^^^^^^^^^^^^^^^^^^^^
Please wrap this in `...' so C-h f can turn it into a hyperlink.
> + doc: /* Return t if SYMBOL is hooked.
^^^
We usually prefer to say "non-nil" for such predicate return value (and
callers shouldn't assume that it's necessarily t).
Stefan
- Re: [PATCH] (Updated) Run hook when variable is set, (continued)
- Re: [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/19
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/19
- [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/20
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/20
- Re: [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/21
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/21
- Re: [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/21
- Re: [PATCH] (Updated) Run hook when variable is set, Stephen J. Turnbull, 2015/02/22
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/22
- [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/22
- Re: [PATCH] (Updated) Run hook when variable is set,
Stefan Monnier <=
- Proposal for debugging/testing option, Kelly Dean, 2015/02/20
- Re: Proposal for debugging/testing option, Stefan Monnier, 2015/02/24
- Re: [PATCH] (Updated) Run hook when variable is set, Johan Bockgård, 2015/02/14
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/15
- Patches: inline vs. attachment, compressed vs. uncompressed. [was: Run hook when variable is set], Alan Mackenzie, 2015/02/15
- Re: [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/06
Re: [PATCH] Run hook when variable is set, Kelly Dean, 2015/02/02