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

[Stefan Monnier]

>>> +#define SYM_CONST 1
>>> +#define SYM_HOOKED 2
>> constant and hooked at the same time is nonsensical, so these aren't two
>> independent bits, instead `vetted' is a 3-valued field.  An `enum'
>> sounds right.
> The compiler will encode the enum into Lisp_Symbol's bitfield, so that will
> produce no change in the compiled code.

I know.  My objection is mostly against the comment(s).
They're basically a left over from when you had 2 separate fields but
are out-of-date w.r.t the current code.

> And SYM_CONST and SYM_HOOKED are better than sprinkling the code with
> 1 and 2 as magic numbers, for the same reason that «false» and «true»
> are better than 0 and 1 for booleans.

I definitely don't want magic constants, which is why I said "An `enum'
sounds right".

> Note that those constant definitions have been there ever since I originally
> combined the constant and hooked fields in the patch I submitted on Feb 9th.

I know.

>>> -static void grow_specpdl (void);
>>> +static inline void grow_specpdl (void);
>> What happens if we don't inline grow_specpdl?
> That's just an optimization since it's in the critical path of specbind, as
> I noted in my message when I made this change on Feb 9th.

But that's orthogonal to the "variable hook" functionality, right?
So I think we can keep this for some later changes.

> I didn't have to. It's just a minor optimization to avoid an extra
> conditional branch before the common case (SYMBOL_PLAINVAL) on the critical

Ah, you're moving the expected common case to the first position.
That sounds OK.  But maybe we can keep this for some later changes, tho.

> Neither would I, if void could never shadow non-void.

It definitely can.  It almost never does, tho.

> I don't see why mark it special, but t and nil are marked special, so
> I decided to do this for the sake of consistency, in case there was some
> reason constants should be marked special.

I'm not sure exactly why t and nil are marked special, but the effect it
has is to prevent their use a lexical vars, and hence prevent their use
as let-bound vars altogether.

The "constant" bit prevents a dynamically scoped (let ((t <foo>)) ...)
while the "special" bit prevents a lexically scoped (let ((t <foo>)) ...).

The first would be dangerous, while the second would be perfectly safe,
but it might be a bit confusing for the programmer who expects t and nil
to always refer to the special predefined constants.

I can't imagine how a chunk of code you try to let-bind the symbol
that's the void-value, marking it "special" is pretty paranoid.

> Because if you could, then as I explained in my previous message, you could
> convert a setq into a makunbound,

I don't think that's a serious problem.

> and since I changed setq to return the override value,

By now, I think I've convinced myself that this an error.

> It would be annoying in the case of (if (setq x y) ...), if you override the
> setting of x but the «if» doesn't respect your choice.

It all depends on what you mean by "override the setting".  In the above
code, the usual intention is "compute y, test the result, and remember
it in x for later reuse".  If the setter function decides to put some
other value into `x', that doesn't mean that the test should also return
another value.

Here's another case:

   (setq x (setq y <foo>))
vs
   (setq y (setq x <foo>))

if you hook `x' and change the value to which it's set, do you really
think the programmer expect the above two expressions to
behave differently?

>> Try M-: (list (setq auto-hscroll-mode 2) auto-hscroll-mode) RET
> Whether or not hooked, you get (2 t).

That's not the point: the `setq' returned the value passed to it, and
not the value to which the variable was set.  This is not done with your
variable hooks, but by some pre-existing code instead, but the principle
is the same.  And in that pre-existing similar situation we made the
opposite choice to the one you're making now.

>>> -  if (sym->constant)
>>> +  if (SYM_CONST_P (sym))
>>> error ("Symbol %s may not be frame-local", SDATA (SYMBOL_NAME (variable)));
>> I think we can keep sym->constant here.
> That would be a bug, since it would signal an error if the symbol is
> hooked. Hooking a symbol must not change the behavior of Emacs.

No, as I said, I'm happy to introduce such "bugs" for frame-local vars.

>> we want to inflict pain on the few rare remaining users of
>> frame-local.
> Just deleting all the code for frame-local would accomplish that
> more effectively.

I've done that in my local branch, and every new release reduces the
support of frame-local vars to some extent.  I haven't thought enough
about when we should finally remove support for them altogether (maybe
25.1 is a bit early still), but in the mean time, there's no reason to
support interaction between them and new features.


        Stefan