Re: Calling Lisp from undo.c's record_* functions

[phillip.lord@russet.org.uk (Phillip Lord)]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> My answers were assuming that we want a solution that keeps using Elisp
> for run_undoable_change.
>
>> That's what Phillip did on his branch.
>
> Yes, I saw that branch, but that's a separate thread.

I just wanted to see if it worked. I don't intend the branch to
necessarily be the right fix. I would prefer not to be moving
functionality back to C if possible.


>>> But of course, in reality it's not the whole record_insert that needs to
>>> be moved, only the run_undoable_change within it.
>>> So if it's difficult to move record_insert to safe spot, maybe we should
>>> take run_undoable_change out of it.
>>> E.g. maybe we could call run_undoable_change from
>>> prepare_to_modify_buffer instead.
>> Beware: prepare_to_modify_buffer is not always called.
>
> When would it not be called?  You mean there are cases where we'd add
> stuff to the undo list but we don't run before-change-functions?
> Wouldn't that be a bug?

Actually, b-c-f is called by prepare_to_modify_buffer_1, so this
conclusion isn't valid.

As far as I can tell, changing a text property will result in changes to
the undo list, but doesn't call prepare_to_modify_buffer. Rather
modify_text_properties appears to call prepare_to_modify_buffer_1
directly. Bit surprising -- I wouldn't have expected a function called
"blah_1" to be called directly from anywhere other than "blah".


>> Why call this at such a low level?
>
> To me, prepare_to_modify_buffer is actually higher-level than record_insert.
>
>> Why not at the level of general_insert_function, Fdelete_region, etc.?
>> (Yes, that would be more places to change, but so what?)
>
> Yes, we could push it to an even higher level, but if
> prepare_to_modify_buffer works, it's preferable, I think.
>
> In any case, these are just suggestions, I don't have strong opinions on
> these issues now ;-)


I've pushed a branch which moves the run_undoable_change functionality
to insdel.c instead. I've moved the entire function which seems to make
more sense to me. All other functionality remains in lisp.

Phil