undo refactoring

undo refactoring

[Phillip Lord]

I thought to move this to emacs-devel rather than #23781 since it's not
directly relevant.


I'll think about the FIXME, that I've just added to undo.c, and see if I
can update that on master. But also, while reading the code again, I've
realised that I don't understand the reason for pre-allocating the
boundary, since when it actually used we do this:


      /* One way or another, cons nil onto the front of the undo list.  */
      if (!NILP (pending_boundary))
	{
	  /* If we have preallocated the cons cell to use here,
	     use that one.  */
	  XSETCDR (pending_boundary, BVAR (current_buffer, undo_list));
	  bset_undo_list (current_buffer, pending_boundary);
	  pending_boundary = Qnil;
	}
      else
	bset_undo_list (current_buffer,
			Fcons (Qnil, BVAR (current_buffer, undo_list)));


Why not just do

	bset_undo_list (current_buffer,
			Fcons (Qnil, BVAR (current_buffer, undo_list)));

and remove all references to pending_boundary (and therefore
`prepare_record` which is now rather badly named)? Or am I missing
something

Phil

Re: undo refactoring

[Stefan Monnier]

> Why not just do

> 	bset_undo_list (current_buffer,
> 			Fcons (Qnil, BVAR (current_buffer, undo_list)));

> and remove all references to pending_boundary (and therefore
> `prepare_record` which is now rather badly named)? Or am I missing
> something

IIUC, the reason for this was so as to avoid an inf-loop when malloc
fails (for lack of memory).

The particular issue was that it was OK for a command to fail because of
malloc failure, but it was not OK for the read-eval-loop itself to fail
before having the opportunity to run another command.

My impression is that this design goal has been ignored for much too
long, so I don't think Emacs behaves this way any more.  And instead we
have now other ways to handle the memory-full situation (such as the
extra memory that's pre-allocated and then released in case the memory
is full).

IOW, I'd be surprised if getting rid of this quirk would ever lead to
a visible change in Emacs's behavior nowadays.


        Stefan

Re: undo refactoring

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> IIUC, the reason for this was so as to avoid an inf-loop when malloc
> fails (for lack of memory).
>
> The particular issue was that it was OK for a command to fail because of
> malloc failure, but it was not OK for the read-eval-loop itself to fail
> before having the opportunity to run another command.

Found the documentation (near the variable rather than the method), and
yes, it's about memory allocation.


/* The first time a command records something for undo.
   it also allocates the undo-boundary object
   which will be added to the list at the end of the command.
   This ensures we can't run out of space while trying to make
   an undo-boundary.  */

That comment was from Richard, and hasn't changed since 1994; I guess
the chance of memory issues from a cons or two are less of a problem
now.

>
> My impression is that this design goal has been ignored for much too
> long, so I don't think Emacs behaves this way any more.  And instead we
> have now other ways to handle the memory-full situation (such as the
> extra memory that's pre-allocated and then released in case the memory
> is full).
>
> IOW, I'd be surprised if getting rid of this quirk would ever lead to
> a visible change in Emacs's behavior nowadays.

I've experimented with this and, at least, all the tests work. I'll try
it out in actual use, then if you are happy make the change on master.


Phil