|
From: | Gregory Heytings |
Subject: | bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced |
Date: | Tue, 03 Jan 2023 16:33:09 +0000 |
I don't even understand what this is supposed to do.Yet you happily threw it away :-(
Because there are no such precautions elsewhere in the code, and the comment above ("In case garbage collection has removed OLD-BUL") does not explain what its purpose is. A few lines below, old-bul is used without such a precaution. Of course, if it has a purpose, it's okay to keep it.
By definition, garbage collection cannot collect something that is still referred to.`garbage-collect` does a bit more than collect garbage. It also truncates undo lists among other things.
Aha. So this means that garbage-collect could be called between (funcall body) and the (while ...) loop two lines below, and could have removed elements that were added to buffer-undo-list by (funcall body), right? If so, that's what the comment should have said.
This one is just plain wrong. It assumes that buffer-undo-list is non-nil initially.AFAICT it warns when the GC's truncation has thrown stuff away (in which case there's a good chance the `apply` element we're building is incorrect).
Given what you write above, that's probably what it meant to do indeed. But it also warns when buffer-undo-list is initially nil, such as with the recipe of this bug report. If adding such a warning is useful, we should check whether old-bul was initially non-nil.
Yes. The current code apparently meant to skip these entries, but it does not work at all. Replacing a broken code that does not work with a clean(er) code that does work seems the right thing.FWIW, I don't see what's cleaner about the new code.
It took me a half hour to figure out what the original code was doing with this 'ap-elt' to which 'buffer-undo-list' is added, and subsequenty modified by the loop below, which does not refer to buffer-undo-list, and which also updates 'old-bul' in an unclear way. I'm biased of course, but I think I would have understood the code in my patch much easier: it does one thing (adding the new undo element) after the other (building the list included in that undo element).
[Prev in Thread] | Current Thread | [Next in Thread] |