Re: [PATCH] (Updated) Run hook when variable is set

[Stefan Monnier <monnier@iro.umontreal.ca>]

>> 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