[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] (Updated) Run hook when variable is set
From: |
Kelly Dean |
Subject: |
Re: [PATCH] (Updated) Run hook when variable is set |
Date: |
Sun, 22 Feb 2015 00:32:02 +0000 |
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.
- Re: The purpose of makunbound, (continued)
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/18
- Re: [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/19
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/19
- [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/20
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/20
- Re: [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/21
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/21
- Re: [PATCH] (Updated) Run hook when variable is set,
Kelly Dean <=
- Re: [PATCH] (Updated) Run hook when variable is set, Stephen J. Turnbull, 2015/02/22
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/22
- [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/22
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/22
- Proposal for debugging/testing option, Kelly Dean, 2015/02/20
- Re: Proposal for debugging/testing option, Stefan Monnier, 2015/02/24
- Re: [PATCH] (Updated) Run hook when variable is set, Johan Bockgård, 2015/02/14
- Re: [PATCH] (Updated) Run hook when variable is set, Stefan Monnier, 2015/02/15
- Patches: inline vs. attachment, compressed vs. uncompressed. [was: Run hook when variable is set], Alan Mackenzie, 2015/02/15
- Re: [PATCH] (Updated) Run hook when variable is set, Kelly Dean, 2015/02/06