[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: master e9e807e: Don't remove notify descriptor that is already gone
From: |
Mattias Engdegård |
Subject: |
Re: master e9e807e: Don't remove notify descriptor that is already gone |
Date: |
Sat, 20 Apr 2019 12:20:15 +0200 |
19 apr. 2019 kl. 16.56 skrev Michael Albinus <address@hidden>:
>
>> (Not only is `auto-revert-notify-watch-descriptor-hash-list' a
>> mouthful, it is a bit misleading. It maps descriptors to lists of
>> buffers. How about `auto-revert--buffers-by-watch-descriptor'?)
>
> No objection.
Thanks, I'll do this separately.
>> Slightly more robust would be to stop reusing descriptors: either made
>> mutable, so that they can be invalidated, or made unique by using a
>> counter. However, the basic design is still somewhat dubious: it tells
>> us whether the descriptor is valid, but that just raises the question:
>> why do we even have to ask? Correct code should understand its own
>> invariants.
>
> In theory you are right. But I fear there could be situations where such
> assumptions do ne keep. A double-check is OK.
What about using the checks in assertions to validate the assumptions, instead
of making the code bug-tolerant? Assertions both inform the reader and check
correctness.
After all, bug-tolerant code just leads to more bugs, to code where the
just-in-case conditions cannot be distinguished from the essential ones, and
where nobody understands the invariants.
>> Now that you `mentioned auto-revert-notify-rm-watch', does it strike
>> you as odd the way it does
>>
>> (maphash
>> (lambda (key value)
>> (when (equal key some-key)
>> do-something))
>> some-hashtable)
>>
>> instead of using the hash table directly? Suggested patch to fix this
>> attached.
>
> I'm still not convinced that we need REMOVE-DESCRIPTOR. We shall always
> remove the descriptor, and assure, that no superfluous events are raised.
Can I take it that you are happy with the patch attached to that message, which
just does away with the maphash?
Regarding getting rid of remove-descriptor, that's fine with me -- I'm
attaching a new patch which does the work in file-notify instead, which is how
I interpret your wish. With that patch, e9e807e931 (addition of the
remove-descriptor parameter) can be reverted.
>> By the way, why don't we give each buffer in auto-revert-mode a unique
>> descriptor, so that the table just maps descriptors to buffers,
>> instead of to lists of buffers? It would simplify the code in many
>> places, and it cannot be that common to have multiple buffers for the
>> same file that it warrants the descriptor-sharing optimisation.
>
> Well, the descriptor is the one we get from filenotify. I don't believe
> we shall do double housekeeping. Sounds to me error-prone.
Agreed, and filenotify seems to ensure that each watch gets a distinct
descriptor even for the same file. I'll prepare a patch for simplifying the
table in autorevert.el, unless you can remember the reason for introducing the
buffer lists in ef3544f6a6. I have read bug#13540, which is mentioned in the
commit message, and am none the wiser.
0001-Make-file-notify-rm-watch-robust-against-reentry.patch
Description: Binary data