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

[Kelly Dean <kelly@prtime.org>]

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.