[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: |
Tue, 16 Apr 2019 15:31:08 +0200 |
16 apr. 2019 kl. 09.04 skrev Michael Albinus <address@hidden>:
> I haven't tested thoroughly yet, but wouldn't it suffice if in
> auto-revert-notify-rm-watch there is just the test
>
> (when (file-notify-valid-p auto-revert-notify-watch-descriptor)
>
> instead of
>
> (when auto-revert-notify-watch-descriptor
Thanks for reading my change. It is a fair question!
First of all, the descriptor wouldn't then be removed from
`auto-revert-notify-watch-descriptor-hash-list' since that part is also guarded
by the condition, but that's just a matter of rearranging code.
(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'?)
I'm not necessarily enamoured with `file-notify-valid-p'. It just tells whether
there is, right now, a descriptor that looks like the argument, in use by
someone, somewhere. These descriptors are reused, making the predicate
dangerous to rely on, or requires brittle code that just knows that no reuse
has occurred.
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.
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.
(For that matter, the documentation doesn't say what mutation is permitted
inside `maphash'. I can guess from the source.)
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.
0001-autorevert.el-auto-revert-notify-rm-watch-Simplify.patch
Description: Binary data