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

[Kelly Dean <kelly@prtime.org>]

Stefan Monnier wrote:
> I think either behavior is equally correct and wrong (sometimes you'd
> rather want one and sometimes you'd rather want the other), so I'd
> rather choose based on which of the two is more efficient/easy to
> implement (and if it's not on the critical path (i.e. the path used when
> the variable is not hooked), then efficiency doesn't matter).

It's not on the critical path, and they're equally efficient and easy to implement. Returning the override value results in a few less lines of code (thus smaller patch), since in some places I don't have to put return statements on separate lines from the calls to run_varhook, and in some places that lets me eliminate a pair of curly brackets too.

> In either case, `setq' should never return Qunbound.

Right. It doesn't.

> Regarding one of the bikesheds,
[snip]
> We can change the name later to something actually better, but for now,
> let's just stick to "constant".

Ok, I'll change it back.

>> +#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. 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.

But I'll put those constants into an enum.

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.

>> -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. The result is a small but measurable performance improvement, at the cost of increasing the executable's size by a few kB (i.e. less than a tenth of a percent, since Emacs is nearly 20MB already).

If that's an undesirable tradeoff, I'll remove the inline.

>> +    case SYMBOL_VARALIAS:
>> +      sym = indirect_variable (sym); XSETSYMBOL (symbol, sym); goto start;
>
> Why did you have to move this?
[snip]
> Hmm... same kind of change here.  I must be missing something.

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 path, in case the compiler implements the switch statement as a series of «if», «else if», «else if» statements. I decided it was worth bothering to make this change because your original reason for wanting to combine the constant and hooked fields was just to avoid an extra conditional branch on this same critical path in set_internal.

Same thing for the other two places (specbind and find_symbol_value) where I made this same change.

These have been here since Feb 9th, and on Feb 7th I explained why.

>> +	      set_internal (symbol, old_value, where, true, Dyn_Unbind);
>
> If using "unbind" when referring to "undoing a let-binding" bothers you,

That doesn't bother me. ‟Unbind” is the right term for that. What bothers me is saying that a variable is ‟unbound” when it's actually bound to void and the binding shadows a non-void binding.

> Personally I have no problem with using "unbind"
> sometimes for "makunbound" and sometimes for "undo a let".

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

> I'd give it a more verbose name, to make sure it doesn't clash with
> someone else's var.  After all, it's not like this is going to be used
> very often, so the length doesn't matter.  `symbol-setter-void-value'
> sounds fine.

Ok, I'll change it.

>> +  XSYMBOL (Vvoid_sentinel)->declared_special = true;
>> +  XSYMBOL (Vvoid_sentinel)->vetted = SYM_CONST;
>
> This value is just a symbol, not a variable, so there's no point marking
> it as special and/or constant.

If the user does something stupid such as (set void-sentinel 'foo), it might as well barf. Especially since the print name looks like a keyword. Thus marked constant.

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.

>> +buf-local: The setter's buffer-local environment.
>> +dyn-local: The innermost dynamic environment in which SYMBOL is bound.
>> +dyn-bind: A new dynamic environment, such as creatable using `let'.
>
> I think this needs clarification, because when I read it, I don't know
> for sure which one I'd get in which case.

Ok, I'll elaborate.

>> +If OLDVAL
>> +is the value of void-sentinel but NEWVAL is not, you can override the new
>> +value, but you can't prevent the variable from being set to a non-void value.
>
> Why not?

Because if you could, then as I explained in my previous message, you could convert a setq into a makunbound, and since I changed setq to return the override value, that means setq could return Qunbound, which is something it should never do.

>> +To hook all variables of a symbol, use `symbol-hook'.
>
> "all variables of a symbol" sounds really odd.

Ok, I'll change it.

> Whether there are
> several variables for a single symbol is really a philosophical
> question,

Until you add multithreading to Emacs. ;-)

> I'd much prefer to keep all the new functions/vars of this feature under
> a common (and new) prefix.  I'll let you choose whether it should be
> `symbol-setter-', `symbol-watcher-`, `symbol-hook-', or whatnot.

Ok, I'll change it.

> Why disallow Qunbound for all but makunbound?
> I understand that it might be strange to return Qunbound in the `setq'
> case, but I'm not sure it's worth the trouble trying to disallow it.

Because I changed setq to return the override value. And as I noted, it also has the fortunate side effect of making (setq x void-sentinel) behave the same regardless of whether x is hooked. IOW, technically, allowing Qunbound for setq would be a bug (even if setq returned its argument, instead of the override value), because hooking a symbol must not change the behavior of Emacs.

> And it seems positively wrong in the case of unwinding the topmost let
> binding of a variable, so even if we do want to disallow it, the test
> shouldn't be "rawenv == Dyn_Makvoid" but "orig_newval == Qunbound".
> So, I think we don't need Dyn_Makvoid (hence we don't need to touch
> Fmakunbound).

Yup, that's a bug. Oops.

> Since writing the above, I think I'm beginning to like the idea of
> (setq <var> <val>) always returning <val> even if <var> ends up set to
> something else.

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.

> Try M-: (list (setq auto-hscroll-mode 2) auto-hscroll-mode) RET

Whether or not hooked, you get (2 t). If hooked, and you override to 3, you get (3 t), not (t t), because the return value of setq is the override value; setq doesn't set the variable and then read it back.

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

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