[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change 0f3c1d
From:
Stefan Monnier
Subject:
Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change 0f3c1db: Changed semantics of first-undoable-change-hook.
Date:
2015年9月29日 01:35:59 -0400
User-agent:
Gnus/5.13 (Gnus v5.13) Emacs/25.0.50 (gnu/linux)
> +(defun undo-needs-boundary-p ()
> + "Returns t if `buffer-undo-list' needs a boundary at the start."
^^^
non-nil
> + ;; buffer-undo-list can be t
> + (listp buffer-undo-list)
> + ;; first element of buffer-undo-list is nil
Please capitalize and punctuate your comments.
> +run_first_undo_hook()
Please always add spaces before the open parens.
> + list = BVAR(current_buffer, undo_list);
Here as well (and ,many other places).
> + if (CONSP (list) && NILP (XCAR (list)))
> + {
> + safe_run_hooks(Qundo_first_undoable_change_hook);
> + }
I think this fails to run the hook when list is nil.
Also, I don't think we need the `safe' version of run-hooks here because
we shouldn't be in a special context like redisplay where signals would
have nasty consequences.
> +
> DEFUN ("undo-boundary", Fundo_boundary, Sundo_boundary, 0, 0, 0,
Try to avoid adding spurious empty lines.
It looks pretty good. Remaining problems I noticed:
- run_first_undo_hook is defined to take no argument, yet it's always
called with `current_buffer' as argument.
- in src/keyboard.c we push undo-boundaries after every command.
This is now made redundant by your code, so you might like to
remove it.
- when trying to remove that code, you'll notice that
Fself_insert_command (and Fdelete_char, IIRC) depends on that code to
know whether the last boundary can be removed (so as to implement the
"bundling").
- It looks like run_first_undo_hook is often called in an "unclean"
state where the C code has set current_buffer rather than calling
set_buffer_internal. This is an optimization that only works when we
know exactly the code that might possibly be run before current_buffer
is set back to its previous value, but it can't be used when running
arbitrary Elisp code. So we should call run_first_undo_hook *before*
setting current_buffer, and run_first_undo_hook should
set_buffer_internal when needed.
Stefan
[Prev in Thread]
Current Thread
[Next in Thread]
- Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change 0f3c1db: Changed semantics of first-undoable-change-hook.,
Stefan Monnier <=