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

[Kelly Dean]

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

If no magic constants, then all the places in trunk that have ⌜->constant = 1⌝ 
must change to ⌜->constant = SYM_CONST⌝ (and my patch already does this, but it 
changes the field name too). That's 9 lines of code.

By retaining the name ⌜constant⌝ for the field name, I can avoid touching 11 
lines of code. But 9 of those are the aforementioned lines, which means if 
magic constants aren't allowed, then retaining the name ⌜constant⌝ saves 
changing just two lines of code. That's a negligible reduction in the size of 
the patch, at the cost of retaining a field name that would no longer be 
appropriate.

Leaving all 11 lines unchanged would leave us with both an inappropriate field 
name and magic constants, and still be only a small reduction in the size of 
the patch.

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

You originally objected to my patch slowing down Emacs. So I looked for 
optimization opportunities to ensure that my patch paid for its performance 
costs. I was successful, and not only did I offset the costs, I even produced a 
net improvement in speed, and provided benchmarks to prove it.

If I remove my optimizations, then my patch will slow down Emacs. And then you 
can once again object to the performance impact.

And the above minor optimization is a trivial change in just two lines of code, 
so removing it would produce a negligible reduction in the size of the patch 
anyway. Removing the even-more-minor optimization that you originally insisted 
on (i.e. combining the constant and hooked fields to save a conditional branch) 
would produce a much larger reduction in the size of the patch, so if you're so 
worried about the patch size, how about we remove the constant-hooked 
combination? My other optimizations, which touch fewer lines, actually improve 
the speed more than the constant-hooked combination does.

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

Same here. Just three occurrences of moving one line of code. And I have to 
move one of those lines anyway, as part of the process of combining the 
constant and hooked fields into one field.

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

Ok, I'll un-mark it as special.

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

The tradeoff is providing a new capability (not previously present in Emacs) 
that's completely unnecessary (even for debugging) and pathological, versus 
strictly abiding by the rule that hooking a symbol must not change the behavior 
of Emacs. Either option is equally easy to implement, and the latter is 
obviously the right one.

>> and since I changed setq to return the override value,
>
> By now, I think I've convinced myself that this an error.

Ok, I'll change it back. Note that changing it back doesn't require enabling 
conversion of setq into makunbound, so I'll still prevent the latter, for the 
sake of correctness.

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

Ok, if the alternative to intentionally introducing a bug is that my patch gets 
rejected, then I'll introduce the bug. But I'll add a comment that it's an 
intentional bug.