An idea: combine-change-calls

An idea: combine-change-calls

[Alan Mackenzie]

Hello, Emacs.

We have the macro combine-after-change-calls, which does more or less
what it says - the executions of after-change-functions is postponed to
the end of the macro, and then they are all executed at once.  This is
all very well, but suffers from the severe restriction that it doesn't
work with before-change-functions at all.

How about combine-change-calls, which combine the b-c-f calls and a-c-f
calls of some function which makes many primitive changes into just one
b-c-f call and one a-c-f call.  It might look something like this:

    (defmacro combine-change-calls (beg end &rest form)
      `(if (not inhibit-modification-hooks)
         (let* ((-beg- ,beg) (-end- ,end)
                (end-marker (copy-marker -end-)))
           (run-hook-with-args before-change-functions beg end)
           (let ((inhibit-modification-hooks t))
             ,@form)
           (run-hook-with-args after-change-functions
                               beg (marker-position end-marker)
                               (- -end- -beg-)))))

.

The motivation is bug #30735, where executing comment-region on a large
region is slow.  It is slow because of the high number of calls to
before/after-change-functions caused by the many individual changes made
by comment-region.

The final part of comment-region could be amended from:

      (save-excursion
        ;; FIXME: maybe we should call uncomment depending on ARG.
        (funcall comment-region-function beg end arg)))

to

      (save-excursion
        ;; FIXME: maybe we should call uncomment depending on ARG.
        (if comment-region-combine-change-calls
            (combine-change-calls beg end
                                  (funcall comment-region-function beg end arg))
          (funcall comment-region-function beg end arg))))

, where comment-region-combine-change-calls would be a new buffer local
variable.

What do people think?

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: An idea: combine-change-calls

[Stefan Monnier]

> The motivation is bug #30735,

[ Not surprised: I told you CC-mode's change-functions are too costly,
  because they presumes that before&after-change-functions are called at
  a "human" rate (comparable to pre/post-command-hook).
  before&after-change-functions should be handled a bit like POSIX
  signals: do as little work as possible there, and handle them
  later elsewhere.  `comment-region` is not the only command that can
  make many small changes.  ]

> What do people think?

I actually do like the idea of combining such things, tho it's risky:
e.g. if the code within combine-change-calls uses syntax-ppss it might
get wrong results since syntax-ppss-flush-cache is triggered via
before-change-functions.  The same problem would affect
syntax-propertize, of course.

Grepping for `add-hook.*before-change-functions` indicates that similar
problem could appear elsewhere.  Not sure what to do about it other than
to say "don't over-use it, it might bite you".

Also we'd need such a system to check that the bounds
are indeed obeyed.

One more thing: with the sample code you showed, undoing will still be
just as slow since it won't benefit from combine-change-calls.
Maybe combine-change-calls should also combine all those changes on the
undo-list into a big "delete+insert" (of course, it could also try and
keep the undo granularity but mark those undo entries so that they're
undone within their own combine-change-calls).


        Stefan

Re: An idea: combine-change-calls

[Alan Mackenzie]

Hello, Stefan.

On Sat, Mar 24, 2018 at 18:18:05 -0400, Stefan Monnier wrote:
> > The motivation is bug #30735,

> [ Not surprised: I told you CC-mode's change-functions are too costly,
>   because they presumes that before&after-change-functions are called at
>   a "human" rate (comparable to pre/post-command-hook).
>   before&after-change-functions should be handled a bit like POSIX
>   signals: do as little work as possible there, and handle them
>   later elsewhere.  `comment-region` is not the only command that can
>   make many small changes.  ]

Yes, acknowledged.

> > What do people think?

> I actually do like the idea of combining such things, tho it's risky:
> e.g. if the code within combine-change-calls uses syntax-ppss it might
> get wrong results since syntax-ppss-flush-cache is triggered via
> before-change-functions.  The same problem would affect
> syntax-propertize, of course.

OK.  That could be a problem in general.

I've actually got a working implementation going.  It is this:

    (defmacro combine-change-calls (beg end &rest form)
      `(if (not inhibit-modification-hooks)
           (let* ((-beg- ,beg) (-end- ,end)
                  (end-marker (copy-marker -end-)))
             (run-hook-with-args 'before-change-functions beg end)
             (let ((inhibit-modification-hooks t))
               ,@form)
             (run-hook-with-args 'after-change-functions
                                 beg (marker-position end-marker)
                                 (- -end- -beg-)))
         ,@form))

With it used in newcomment.el, C-c C-c and C-u C-c C-c on large portions
of CC Mode files go fast enough.

> Grepping for `add-hook.*before-change-functions` indicates that similar
> problem could appear elsewhere.  Not sure what to do about it other than
> to say "don't over-use it, it might bite you".

> Also we'd need such a system to check that the bounds
> are indeed obeyed.

> One more thing: with the sample code you showed, undoing will still be
> just as slow since it won't benefit from combine-change-calls.

Yes, this is indeed the case.

> Maybe combine-change-calls should also combine all those changes on the
> undo-list into a big "delete+insert" (of course, it could also try and
> keep the undo granularity but mark those undo entries so that they're
> undone within their own combine-change-calls).

:-)  Either of those would be quite a project, but possibly worth doing.
Thanks for the idea.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: An idea: combine-change-calls

[Stefan Monnier]

> I've actually got a working implementation going.  It is this:
>
>     (defmacro combine-change-calls (beg end &rest form)
>       `(if (not inhibit-modification-hooks)
>            (let* ((-beg- ,beg) (-end- ,end)
>                   (end-marker (copy-marker -end-)))
>              (run-hook-with-args 'before-change-functions beg end)
>              (let ((inhibit-modification-hooks t))
>                ,@form)
>              (run-hook-with-args 'after-change-functions
>                                  beg (marker-position end-marker)
>                                  (- -end- -beg-)))
>          ,@form))

You need to evaluate `beg` and `end` even if inhibit-modification-hooks
is set, otherwise someone will get bitten.

I recommend you move the `form` to a lambda so you don't have to
duplicate it:

    `(let ((body (lambda () ,@form))
           (-beg- ,beg)
           (-end- ,end))
       ...)

Another benefit is that by moving `form` outside of the `let*`, you
won't need to use gensym/make-symbol nor obfuscated names.

I'd also recommend you check that `beg` hasn't changed position and that
the distance between end-marker and point-max remained the same.

>> Maybe combine-change-calls should also combine all those changes on the
>> undo-list into a big "delete+insert" (of course, it could also try and
>> keep the undo granularity but mark those undo entries so that they're
>> undone within their own combine-change-calls).
> :-)  Either of those would be quite a project, but possibly worth doing.

Replacing the entries with a pair of delete+insert should be
pretty easy.  Something like

    (let ((old-buffer-undo-list buffer-undo-list)
          (orig-text (buffer-substring beg end)))
      ...
      (setq buffer-undo-list
            `((,(marker-position end-marker) ,beg)
              (,orig-text . ,beg)
              . ,old-buffer-undo-list)))

modulo sanity checks (i.e. don't do it if undo is disabled and don't do
it if old-buffer-undo-list is not within buffer-undo-list any more).


        Stefan

Re: An idea: combine-change-calls

[Alan Mackenzie]

Hello, Stefan.

On Sun, Mar 25, 2018 at 16:05:40 -0400, Stefan Monnier wrote:
> > I've actually got a working implementation going.  It is this:

> >     (defmacro combine-change-calls (beg end &rest form)
> >       `(if (not inhibit-modification-hooks)
> >            (let* ((-beg- ,beg) (-end- ,end)
> >                   (end-marker (copy-marker -end-)))
> >              (run-hook-with-args 'before-change-functions beg end)
> >              (let ((inhibit-modification-hooks t))
> >                ,@form)
> >              (run-hook-with-args 'after-change-functions
> >                                  beg (marker-position end-marker)
> >                                  (- -end- -beg-)))
> >          ,@form))

> You need to evaluate `beg` and `end` even if inhibit-modification-hooks
> is set, otherwise someone will get bitten.

> I recommend you move the `form` to a lambda so you don't have to
> duplicate it:

>     `(let ((body (lambda () ,@form))
>            (-beg- ,beg)
>            (-end- ,end))
>        ...)

> Another benefit is that by moving `form` outside of the `let*`, you
> won't need to use gensym/make-symbol nor obfuscated names.

> I'd also recommend you check that `beg` hasn't changed position and that
> the distance between end-marker and point-max remained the same.

> >> Maybe combine-change-calls should also combine all those changes on the
> >> undo-list into a big "delete+insert" (of course, it could also try and
> >> keep the undo granularity but mark those undo entries so that they're
> >> undone within their own combine-change-calls).
> > :-)  Either of those would be quite a project, but possibly worth doing.

> Replacing the entries with a pair of delete+insert should be
> pretty easy.  Something like

>     (let ((old-buffer-undo-list buffer-undo-list)
>           (orig-text (buffer-substring beg end)))
>       ...
>       (setq buffer-undo-list
>             `((,(marker-position end-marker) ,beg)
>               (,orig-text . ,beg)
>               . ,old-buffer-undo-list)))

> modulo sanity checks (i.e. don't do it if undo is disabled and don't do
> it if old-buffer-undo-list is not within buffer-undo-list any more).

I'm experimenting with a different strategy: surrounding the mass of
elements in buffer-undo-list with a `(combine-change-begin ,beg ,end)
and a `(combine-change-end ,beg ,end).  This is less violent to the undo
mechanism, for example, still permitting programs to analyse the undo
list.

primitive-undo, when it meets the latter of these, calls
before-change-functions, binds inhibit-modification-hooks to t and calls
itself recursively.  This recursive invocation is terminated by the
combine-change-begin, after-change-functions being called immediately on
return.

The two arms inserted into the pcase form in primitive-undo look like:

          (`(combine-change-end
             ,(and beg (pred integerp))
             ,(and end (pred integerp)))
           (save-excursion
             (run-hook-with-args 'before-change-functions beg end))
           (setq old-len (- end beg))
           (let ((inhibit-modification-hooks t))
             (setq list (primitive-undo 1 list))))
          (`(combine-change-begin
             ,(and beg (pred integerp))
             ,(and end (pred integerp)))
           (if old-len
               ;; Non-nested invocation of `primitive-undo'.
               (save-excursion
                 (run-hook-with-args 'after-change-functions beg end old-len)
                 (setq old-len nil))
             ;; Nested invocation of `primitive-undo'.  Push the element back
             ;; on the list, and push nil to terminate this invocation.
             (push next list)
             (push nil list)))

(where `old-len' is an extra local variable bound to nil in the
surrounding `let' form).

The current version of combine-change-calls, incorporating (at least
most of) your suggestions now looks like:

   (defmacro combine-change-calls (beg end &rest form)
      `(let* ((-beg- ,beg)
              (-end- ,end)
              (body (lambda () ,@form))
              (end-marker (copy-marker -end-)))
         (if inhibit-modification-hooks
             (funcall body)
           (run-hook-with-args 'before-change-functions -beg- -end-)
           (unless (eq buffer-undo-list t)
             (push `(combine-change-begin ,-beg- ,-end-) buffer-undo-list))
           (unwind-protect
               (let ((inhibit-modification-hooks t))
                 (funcall body))
             (unless (eq buffer-undo-list t)
               (push `(combine-change-end ,-beg- ,(marker-position end-marker))
                     buffer-undo-list)))
           (run-hook-with-args 'after-change-functions
                               beg (marker-position end-marker)
                               (- -end- -beg-)))))
 
This makes undo blindingly fast after a large comment-region operation.
It doesn't always leave point in the right place (I understand why -
it's the C function record_point failing to record point because the top
element of buffer-undo-list is no longer nil; it's a
combine-change-begin list).

Do you have any more helpful suggestions for this idea?

Basically, the combine-change-calls idea works.  Given enough
encouragement, I will get my disorganised changes into a proper patch
with documentation, with a view to pushing it to master.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: An idea: combine-change-calls

[Stefan Monnier]

> I'm experimenting with a different strategy: surrounding the mass of
> elements in buffer-undo-list with a `(combine-change-begin ,beg ,end)
> and a `(combine-change-end ,beg ,end).

Beware: this changes the format of entries that can appear in the
buffer-undo-list, which has repercussions beyond primitive-undo, IOW it
can/will break other things such as undo-in-region and undo-tree.

Better use the (apply DELTA BEG END FUN-NAME . ARGS) form, which was
introduced specifically for use of such extensions.


        Stefan

Re: An idea: combine-change-calls

[Stefan Monnier]

> This makes undo blindingly fast after a large comment-region operation.
> It doesn't always leave point in the right place (I understand why -
> it's the C function record_point failing to record point because the top
> element of buffer-undo-list is no longer nil; it's a
> combine-change-begin list).

You can easily fix this by explicitly pushing the position of point onto
buffer-undo-list at the beginning.

> Basically, the combine-change-calls idea works.  Given enough
> encouragement, I will get my disorganised changes into a proper patch
> with documentation, with a view to pushing it to master.

Go, go, go, you can do it!


        Stefan

Re: An idea: combine-change-calls

[Stefan Monnier]

>    (defmacro combine-change-calls (beg end &rest form)
>       `(let* ((-beg- ,beg)
>               (-end- ,end)
>               (body (lambda () ,@form))

Don't use `let*` here since otherwise you have a variable capture if
`form` uses variables with name -beg- or -end-, or if the expression
`end` uses a variable named -beg-.

[ and a nitpick: `form` is usually used for a single expression whereas
  here it holds a list of expressions, so I'd call it `forms`.  ]


        Stefan

Re: An idea: combine-change-calls

[Alan Mackenzie]

Hello, Stefan.

On Mon, Mar 26, 2018 at 17:07:40 -0400, Stefan Monnier wrote:
> > I'm experimenting with a different strategy: surrounding the mass of
> > elements in buffer-undo-list with a `(combine-change-begin ,beg ,end)
> > and a `(combine-change-end ,beg ,end).

> Beware: this changes the format of entries that can appear in the
> buffer-undo-list, which has repercussions beyond primitive-undo, IOW it
> can/will break other things such as undo-in-region and undo-tree.

Thanks.  I didn't know about undo-in-region.  That is a peculiarly badly
documented feature: Neither manual states what it means for an
undo-entry to be "within the region", and nowhere is it stated what
happens to undo-entries which aren't in the region.  Maybe they are
just discarded, maybe they are somehow kept in the undo list.

It looks like I've got some source code to read.

By the way, what's undo-tree?  I've not been able to find that symbol at
all in the source code.

> Better use the (apply DELTA BEG END FUN-NAME . ARGS) form, which was
> introduced specifically for use of such extensions.

This won't work, at least not without some seriously twisted coding: The
essential thing about (combine-change-end/begin ..) is that they bind
inhibit-modification-hooks to non-nil for other entries in the
undo-list.  Maybe FUN-NAME could call primitive-undo, but this doesn't
seem wise.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: An idea: combine-change-calls

[Alan Mackenzie]

Hello, Stefan.

On Mon, Mar 26, 2018 at 20:36:15 -0400, Stefan Monnier wrote:
> >    (defmacro combine-change-calls (beg end &rest form)
> >       `(let* ((-beg- ,beg)
> >               (-end- ,end)
> >               (body (lambda () ,@form))

> Don't use `let*` here since otherwise you have a variable capture if
> `form` uses variables with name -beg- or -end-, or if the expression
> `end` uses a variable named -beg-.

Good point.  I will delete that obtrusive asterisk.

> [ and a nitpick: `form` is usually used for a single expression whereas
>   here it holds a list of expressions, so I'd call it `forms`.  ]

I'll do that, too.

Thanks.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: An idea: combine-change-calls

[Stefan Monnier]

> happens to undo-entries which aren't in the region.  Maybe they are
> just discarded, maybe they are somehow kept in the undo list.

They're just discarded (when building the list to pass to
primitive-undo only: they stay in buffer-undo-list, of course).

> By the way, what's undo-tree?  I've not been able to find that symbol at
> all in the source code.

See http://elpa.gnu.org/packages/undo-tree.html

>> Better use the (apply DELTA BEG END FUN-NAME . ARGS) form, which was
>> introduced specifically for use of such extensions.
> This won't work, at least not without some seriously twisted coding: The
> essential thing about (combine-change-end/begin ..) is that they bind
> inhibit-modification-hooks to non-nil for other entries in the
> undo-list.  Maybe FUN-NAME could call primitive-undo, but this doesn't
> seem wise.

Actually the way I was thinking of using it was something like:

    (let ((elem-apply `(apply 0 ,beg ,end ,#'my-undo-combining nil)))
      (push elem-apply buffer-undo-list)
      (funcall body)
      (let ((new-bul (memq elem-apply buffer-undo-list)))
        (when new-bul
          (let ((undo-elems buffer-undo-list))
            (setf (nthcdr (- (length undo-elems) (length new-bul))
                          undo-elems)
                  nil)
            (setf (nth 1 elem-apply) (- end-marker end))
            (setf (nth 3 elem-apply) (marker-position end-marker))
            (setf (nth 5 elem-apply) undo-elems)
            (setq buffer-undo-list new-bul)))))

and then

    (defun my-undo-combining (undo-elems)
      (let ((inhibit-modification-hooks t))
        (while t
          (primitive-undo 1 undo-elems))))

But you might prefer just replacing the whole thing with a pair of
insert+delete, which is simpler and vastly more efficient (but with the
disadvantage that it doesn't preserve markers quite as well).


        Stefan

Re: An idea: combine-change-calls

[Alan Mackenzie]

Hello again, Stefan.

On Tue, Mar 27, 2018 at 14:30:53 -0400, Stefan Monnier wrote:
> > happens to undo-entries which aren't in the region.  Maybe they are
> > just discarded, maybe they are somehow kept in the undo list.

> They're just discarded (when building the list to pass to
> primitive-undo only: they stay in buffer-undo-list, of course).

OK.

> > By the way, what's undo-tree?  I've not been able to find that symbol at
> > all in the source code.

> See http://elpa.gnu.org/packages/undo-tree.html

Interesting.  But I think it would be overkill for me personally.

> >> Better use the (apply DELTA BEG END FUN-NAME . ARGS) form, which was
> >> introduced specifically for use of such extensions.
> > This won't work, at least not without some seriously twisted coding: The
> > essential thing about (combine-change-end/begin ..) is that they bind
> > inhibit-modification-hooks to non-nil for other entries in the
> > undo-list.  Maybe FUN-NAME could call primitive-undo, but this doesn't
> > seem wise.

> Actually the way I was thinking of using it was something like:

>     (let ((elem-apply `(apply 0 ,beg ,end ,#'my-undo-combining nil)))
>       (push elem-apply buffer-undo-list)
>       (funcall body)
>       (let ((new-bul (memq elem-apply buffer-undo-list)))
>         (when new-bul
>           (let ((undo-elems buffer-undo-list))
>             (setf (nthcdr (- (length undo-elems) (length new-bul))
>                           undo-elems)
>                   nil)
>             (setf (nth 1 elem-apply) (- end-marker end))
>             (setf (nth 3 elem-apply) (marker-position end-marker))
>             (setf (nth 5 elem-apply) undo-elems)
>             (setq buffer-undo-list new-bul)))))

> and then

>     (defun my-undo-combining (undo-elems)
>       (let ((inhibit-modification-hooks t))
>         (while t
>           (primitive-undo 1 undo-elems))))

OK, I get the general idea: there's a recursive call to primitive-undo
from the FUN-NAME in the `apply' undo element.

But it's actually more complicated still: When the undo is in progress,
FUN-NAME must push (a) new `apply' element(s) to buffer-undo-list for
the use of a possible redo.  This is probably possible, but my head's
beginning to hurt at the moment.  ;-)

My scheme (of introducing new types of element to the undo list) is
quite a bit simpler, but has the disadvantage of an incompatible change
in the undo list format.  I accept that this disadvantage is severe.

I think I will continue to refine my scheme to get practice and
experience.  Then will be the time to decide on replacing it with the
above `apply' scheme.

> But you might prefer just replacing the whole thing with a pair of
> insert+delete, which is simpler and vastly more efficient (but with the
> disadvantage that it doesn't preserve markers quite as well).

I really don't want to do this.  Some people will want to analyse
buffer-undo-list and such a replacement will throw off this analysis,
possibly to the extent of making it useless.  In practice, wrapping the
original undo elements in what we've been talking about is easily fast
enough.  (An undo over ~2000 lines of comment-region'd C++ code was
taking ~0.05s (if I remember correctly), though displaying it after that
took appreciably longer.)

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: An idea: combine-change-calls

[Stefan Monnier]

>> See http://elpa.gnu.org/packages/undo-tree.html
> Interesting.  But I think it would be overkill for me personally.

The point is not to make you use it, but that this package uses the same
buffer-undo-list, so any change to the format would break it.

> I really don't want to do this.  Some people will want to analyse
> buffer-undo-list and such a replacement will throw off this analysis,
> possibly to the extent of making it useless.

How/why?  I can't think of any case where it would cause such problems:
the resulting undo-list, tho less detailed than the original one, is
perfectly valid.


        Stefan

Re: An idea: combine-change-calls

[Alan Mackenzie]

Hello, Stefan.

On Tue, Mar 27, 2018 at 16:24:28 -0400, Stefan Monnier wrote:
> >> See http://elpa.gnu.org/packages/undo-tree.html
> > Interesting.  But I think it would be overkill for me personally.

> The point is not to make you use it, but that this package uses the same
> buffer-undo-list, so any change to the format would break it.

It would need amendment, of course, but that wouldn't be difficult.  But
I suspect that the mechanism you suggested (an `apply' format element
recursively calling primitive-undo), will break other packages too, even
if the format of undo lists isn't changed.  We'd have to try it out.

> > I really don't want to do this [combining all the undo elements into
> > a single undo element].  Some people will want to analyse
> > buffer-undo-list and such a replacement will throw off this
> > analysis, possibly to the extent of making it useless.

> How/why?  I can't think of any case where it would cause such problems:
> the resulting undo-list, tho less detailed than the original one, is
> perfectly valid.

You put it most eloquently yourself in Subject: Re: What improvements
would be truly useful?  Date: Thu, 08 Mar 2018 23:43:57 -0500:

    Of course Emacs can also hide information (as text-properties, as
    invisible text, as data stored in buffer-local variables, ...) but most
    packages follow a design where as little info as possible is hidden.
    Indeed, whenever I hide such information, I think it over many times
    because I know there's a very strong chance that users won't like it.

What we've been discussing goes beyond hiding information, it is the
destruction of information.  Users, maybe just a few, won't like that at
all.

Incidentally, position elements in the undo list don't work: `undo'
removes them from buffer-undo-list.  I think you amended that bit of
code some years ago.  Can you say why this is done?  The comment in the
code:

    ;; Don't specify a position in the undo record for the undo command.
    ;; Instead, undoing this should move point to where the change is.

doesn't give any reason, and the various pertinent commit messages
aren't any help either.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: An idea: combine-change-calls

[Stefan Monnier]

> It would need amendment, of course, but that wouldn't be difficult.

I'd rather try and avoid that.  And if we really do want to extent the
format because we consider that (apply ...) is not good enough here,
than I'd want this new extension to be generic rather than specific for
this particular use-case.

> But I suspect that the mechanism you suggested (an `apply' format
> element recursively calling primitive-undo), will break other packages
> too, even if the format of undo lists isn't changed.

Could be, indeed.

> What we've been discussing goes beyond hiding information, it is the
> destruction of information.  Users, maybe just a few, won't like that at
> all.

Not at all.  It's just optimizing the representation of the undo-log.
If there's an undo-boundary in there, then indeed, we'd be throwing away
a bit of information, but I assumed we wouldn't care about that case.

Whatever you decide to do with the undo-log, handling undo-boundary
pushed during the execution of `body` will be tricky I suspect (except
if we just don't touch the undo-list, of course).

> Incidentally, position elements in the undo list don't work: `undo'
> removes them from buffer-undo-list.

Are you sure they "don't work" (they seemed to work in my test)?

IIUC The code you cite only strips them from the undo elements added
while performing an undo (i.e. from "redo" elements), so they should
still work for a plain "edit .... undo".

> I think you amended that bit of code some years ago.  Can you say why
> this is done?  The comment in the code:
>
>     ;; Don't specify a position in the undo record for the undo command.
>     ;; Instead, undoing this should move point to where the change is.
>
> doesn't give any reason, and the various pertinent commit messages
> aren't any help either.

Hmm... good question.  I see this code basically dates back to

  commit 2512c9f0f0e6cc71c601ffdb0690b9cf5642734b
  Author: Richard M. Stallman <rms@gnu.org>
  Date:   Wed Mar 16 23:41:32 1994 +0000

    (undo): Don't let the undo entries for the undo
    contain a specific buffer position.  Delete it if there is one.

and no, I don't know why we do this.


        Stefan

Re: An idea: combine-change-calls

[Alan Mackenzie]

Hello, Stefan.

On Wed, Mar 28, 2018 at 17:26:30 -0400, Stefan Monnier wrote:
> > It would need amendment, of course, but that wouldn't be difficult.

> I'd rather try and avoid that.  And if we really do want to extent the
> format because we consider that (apply ...) is not good enough here,
> ....

I don't think "consider" is the right word here.  I don't think it will
work at all.  In primitive-undo, some undo list is an argument, and it
has elements removed from it and it is then the return value.  If we try
to call primitive-undo recursively through an (apply ...) form, there is
no interface to return the depleted list to the calling p-u.  We could,
of course, use some global variables instead of arguments, but this
would involve changes just as incompatible as making additions to the
undo list elements.

> ... than I'd want this new extension to be generic rather than
> specific for this particular use-case.

It is generic, in the sense it handles any case where
before/after-change-functions are to be condensed into one call of each.

What do you mean by generic, here?

[ .... ]

> > What we've been discussing goes beyond hiding information, it is the
> > destruction of information.  Users, maybe just a few, won't like
> > that at all.

> Not at all.  It's just optimizing the representation of the undo-log.
> If there's an undo-boundary in there, then indeed, we'd be throwing away
> a bit of information, but I assumed we wouldn't care about that case.

It does a good deal more than "optimizing the representation" - it makes
an irreversible change which loses information.  Somebody, sometime, is
going to need that info.

> Whatever you decide to do with the undo-log, handling undo-boundary
> pushed during the execution of `body` will be tricky I suspect (except
> if we just don't touch the undo-list, of course).

In my current code, the only undo-boundary pushed (in the handling of
combine-change-begin) is immediately acted upon to terminate the
recursive invocation of primitive-undo.  This is pushed onto the LIST
variable in the nested p-u, and doesn't affect buffer-undo-list or
pending-undo-list.

> > Incidentally, position elements in the undo list don't work: `undo'
> > removes them from buffer-undo-list.

> Are you sure they "don't work" (they seemed to work in my test)?

> IIUC The code you cite only strips them from the undo elements added
> while performing an undo (i.e. from "redo" elements), so they should
> still work for a plain "edit .... undo".

Ah, is that it?  I had some difficulty understanding it properly.

> > I think you amended that bit of code some years ago.  Can you say why
> > this is done?  The comment in the code:

> >     ;; Don't specify a position in the undo record for the undo command.
> >     ;; Instead, undoing this should move point to where the change is.

> > doesn't give any reason, and the various pertinent commit messages
> > aren't any help either.

> Hmm... good question.  I see this code basically dates back to

>   commit 2512c9f0f0e6cc71c601ffdb0690b9cf5642734b
>   Author: Richard M. Stallman <rms@gnu.org>
>   Date:   Wed Mar 16 23:41:32 1994 +0000

>     (undo): Don't let the undo entries for the undo
>     contain a specific buffer position.  Delete it if there is one.

Yes.

> and no, I don't know why we do this.

Thinking about it, that comment above ("Don't specify a position ....")
reads as if it was originally in place on some code which added elements
to buffer-undo-list, and then got hurredly moved to `undo' when the
strategy was changed to delete such elements.  Again, it isn't clear why
position elements get deleted.  Any code which adds them (such as my new
code) will have a reason for doing so.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: An idea: combine-change-calls

[Stefan Monnier]

> I don't think "consider" is the right word here.  I don't think it will
> work at all.

I expect otherwise.

> In primitive-undo, some undo list is an argument, and it
> has elements removed from it and it is then the return value.  If we try
> to call primitive-undo recursively through an (apply ...) form, there is
> no interface to return the depleted list to the calling p-u.

You keep assuming a shape like

    ...previous elements...
    (apply ....)
    ...undo-elements...
    (apply ....)
    ...subsequent elements...

where I'm assuming a shape like:

    ...previous elements...
    (apply ....  ...undo-elements...)
    ...subsequent elements...

I don't see any part of primitive-undo which would prevent it being used
recursively in such a situation.

>> ... than I'd want this new extension to be generic rather than
>> specific for this particular use-case.
> It is generic, in the sense it handles any case where
> before/after-change-functions are to be condensed into one call of each.
> What do you mean by generic, here?

That it can be used by other things than combine-change-calls.
I.e. generic is the same sense as the (apply ....) thingy is generic.

> It does a good deal more than "optimizing the representation" - it makes
> an irreversible change which loses information.

To the extent that most execution of code makes irreversible changes,
I agree, but other than that, I fail to see what information you're
thinking about.

> Somebody, sometime, is going to need that info.

Could you give some hypothetical example to give me an idea of what kind
of info you're thinking of and where/when it might be needed?

>> Whatever you decide to do with the undo-log, handling undo-boundary
>> pushed during the execution of `body` will be tricky I suspect (except
>> if we just don't touch the undo-list, of course).
> In my current code, the only undo-boundary pushed (in the handling of
> combine-change-begin) is immediately acted upon to terminate the
> recursive invocation of primitive-undo.  This is pushed onto the LIST
> variable in the nested p-u, and doesn't affect buffer-undo-list or
> pending-undo-list.

I'm referring to undo-boundaries pushed by the "execution of
`body`", not by your code.

IIUC we agree that this is considered an unimportant use-case and it's
OK to just ignore such boundaries.

>> IIUC The code you cite only strips them from the undo elements added
>> while performing an undo (i.e. from "redo" elements), so they should
>> still work for a plain "edit .... undo".
> Ah, is that it?  I had some difficulty understanding it properly.

Yes, that's it.  I don't think it affects this discussion at all.


        Stefan

Re: An idea: combine-change-calls

[Alan Mackenzie]

Hello, Stefan.

On Thu, Mar 29, 2018 at 11:40:26 -0400, Stefan Monnier wrote:
> > I don't think "consider" is the right word here.  I don't think it will
> > work at all.

> I expect otherwise.

[ .... ]

> You keep assuming a shape like

>     ...previous elements...
>     (apply ....)
>     ...undo-elements...
>     (apply ....)
>     ...subsequent elements...

> where I'm assuming a shape like:

>     ...previous elements...
>     (apply ....  ...undo-elements...)
>     ...subsequent elements...

I think you've just drawn two pictures of possible undo lists.

> I don't see any part of primitive-undo which would prevent it being used
> recursively in such a situation.

Right, I think I've got it now.  The macro combine-change-calls would
maybe first push some sort of sentinal element onto buffer-undo-list,
then let ,@forms run, then massage the list so that the 2000 elements
created by ,@forms would become part of the `apply' element.

Yes, that would work.  :-)

I will implement this.

> >> ... than I'd want this new extension to be generic rather than
> >> specific for this particular use-case.
> > It is generic, in the sense it handles any case where
> > before/after-change-functions are to be condensed into one call of each.
> > What do you mean by generic, here?

> That it can be used by other things than combine-change-calls.
> I.e. generic is the same sense as the (apply ....) thingy is generic.

I still don't understand.  What other things could possibly be similiar
enough to collecting before/after-change-functions to be abstractable
into some sort of parameter?  Could you give an example?

> > It does a good deal more than "optimizing the representation" - it makes
> > an irreversible change which loses information.

> To the extent that most execution of code makes irreversible changes,
> I agree, but other than that, I fail to see what information you're
> thinking about.

> > Somebody, sometime, is going to need that info.

> Could you give some hypothetical example to give me an idea of what kind
> of info you're thinking of and where/when it might be needed?

The information is the original undo entries created by the Emacs
primitives whilst in combine-change-calls.

As to where/when/why it might be needed, I couldn't say at this point.
Somebody, sometime, will want it for some reason, just like you said in
your piece about "hiding information" I quoted a short while ago.

Why are you so keen to destroy this detailed information?  Its continued
existence does not cause any disadvantage.  The run time which would be
saved in `undo' is minimal and inconsequential.

> >> Whatever you decide to do with the undo-log, handling undo-boundary
> >> pushed during the execution of `body` will be tricky I suspect (except
> >> if we just don't touch the undo-list, of course).
> > In my current code, the only undo-boundary pushed (in the handling of
> > combine-change-begin) is immediately acted upon to terminate the
> > recursive invocation of primitive-undo.  This is pushed onto the LIST
> > variable in the nested p-u, and doesn't affect buffer-undo-list or
> > pending-undo-list.

> I'm referring to undo-boundaries pushed by the "execution of
> `body`", not by your code.

OK.  We could amend primitive-undo such that the argument N could,
additionally, take the value t, meaning "undo all elements in the list,
ignoring nils".  Or something like that.

> IIUC we agree that this is considered an unimportant use-case and it's
> OK to just ignore such boundaries.

I suppose so.  What would such a boundary mean anyway?  We're talking
about a scenario where all the change hook invocations are amalgamated.
How could it make sense to split that up?

> >> IIUC The code you cite only strips them from the undo elements added
> >> while performing an undo (i.e. from "redo" elements), so they should
> >> still work for a plain "edit .... undo".
> > Ah, is that it?  I had some difficulty understanding it properly.

> Yes, that's it.  I don't think it affects this discussion at all.

I suppose not.  Not at the moment, anyway.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: An idea: combine-change-calls

[Stefan Monnier]

> Right, I think I've got it now.  The macro combine-change-calls would
> maybe first push some sort of sentinal element onto buffer-undo-list,
> then let ,@forms run, then massage the list so that the 2000 elements
> created by ,@forms would become part of the `apply' element.

Right.  Now you can re-read the pseudo-code I had sent:

    (let ((elem-apply `(apply 0 ,beg ,end ,#'my-undo-combining nil)))
      (push elem-apply buffer-undo-list)
      (funcall body)
      (let ((new-bul (memq elem-apply buffer-undo-list)))
        (when new-bul
          (let ((undo-elems buffer-undo-list))
            (setf (nthcdr (- (length undo-elems) (length new-bul))
                          undo-elems)
                  nil)
            (setf (nth 1 elem-apply) (- end-marker end))
            (setf (nth 3 elem-apply) (marker-position end-marker))
            (setf (nth 5 elem-apply) undo-elems)
            (setq buffer-undo-list new-bul)))))

and then

    (defun my-undo-combining (undo-elems)
      (let ((inhibit-modification-hooks t))
        (while t
          (primitive-undo 1 undo-elems))))

>> That it can be used by other things than combine-change-calls.
>> I.e. generic is the same sense as the (apply ....) thingy is generic.
> I still don't understand.  What other things could possibly be similiar
> enough to collecting before/after-change-functions to be abstractable
> into some sort of parameter?  Could you give an example?

I could imagine a macro `with-inhibit-read-only` which would bind
inhibit-read-only to t and then tweak the buffer-undo-list similarly to
what you're doing such that the undo is also performed with
inhibit-read-only bound to t.

>> Could you give some hypothetical example to give me an idea of what kind
>> of info you're thinking of and where/when it might be needed?
> The information is the original undo entries created by the Emacs
> primitives whilst in combine-change-calls.
> As to where/when/why it might be needed, I couldn't say at this point.
> Somebody, sometime, will want it for some reason, just like you said in
> your piece about "hiding information" I quoted a short while ago.
> Why are you so keen to destroy this detailed information?

I'm not interested in destroying this information.  I'm just trying to
solve the problem at hand and it's a simple and efficient solution, and
neither you nor I can come up with a scenario where this simpler
solution is worse, apparently.

> Its continued existence does not cause any disadvantage.  The run time
> which would be saved in `undo' is minimal and inconsequential.

Agreed, the gain is in the simplicity of combine-change-calls.

>> I'm referring to undo-boundaries pushed by the "execution of
>> `body`", not by your code.
> OK.  We could amend primitive-undo such that the argument N could,
> additionally, take the value t, meaning "undo all elements in the list,
> ignoring nils".  Or something like that.

We don't need that: can call it repeatedly until it returns nil.

>> IIUC we agree that this is considered an unimportant use-case and it's
>> OK to just ignore such boundaries.
> I suppose so.  What would such a boundary mean anyway?  We're talking
> about a scenario where all the change hook invocations are amalgamated.
> How could it make sense to split that up?

Some commands explicitly push undo boundaries in the middle of their
execution, so that the user can easily undo the 2nd half of the
command's execution, for example.

This is rare enough that I think it's perfectly OK to say that such
use-cases are not supported by combine-change-calls.  [ It's also the
only use-case I could think of where using a simple pair of
"delete+insert" undo entries would have made a difference.  ]


        Stefan

Re: An idea: combine-change-calls

[Johan Bockgård]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I'm experimenting with a different strategy: surrounding the mass of
>> elements in buffer-undo-list with a `(combine-change-begin ,beg ,end)
>> and a `(combine-change-end ,beg ,end).
>
> Beware: this changes the format of entries that can appear in the
> buffer-undo-list, which has repercussions beyond primitive-undo, IOW it
> can/will break other things such as undo-in-region and undo-tree.

But the `(apply ...)' forms are a problem for exactly these other
things. It's too powerful. The packages that manipulate buffer-undo-list
can't (and don't try to) handle `apply' properly, since it can contain
arbitrary effects.

> Better use the (apply DELTA BEG END FUN-NAME . ARGS) form, which was
> introduced specifically for use of such extensions.

Re: An idea: combine-change-calls

[Alan Mackenzie]

Hello, Stefan.

On Thu, Mar 29, 2018 at 15:10:47 -0400, Stefan Monnier wrote:
> > Right, I think I've got it now.  The macro combine-change-calls would
> > maybe first push some sort of sentinal element onto buffer-undo-list,
> > then let ,@forms run, then massage the list so that the 2000 elements
> > created by ,@forms would become part of the `apply' element.

> Right.  Now you can re-read the pseudo-code I had sent:

>     (let ((elem-apply `(apply 0 ,beg ,end ,#'my-undo-combining nil)))
>       (push elem-apply buffer-undo-list)
>       (funcall body)
>       (let ((new-bul (memq elem-apply buffer-undo-list)))
>         (when new-bul
>           (let ((undo-elems buffer-undo-list))
>             (setf (nthcdr (- (length undo-elems) (length new-bul))
>                           undo-elems)
>                   nil)
>             (setf (nth 1 elem-apply) (- end-marker end))
>             (setf (nth 3 elem-apply) (marker-position end-marker))
>             (setf (nth 5 elem-apply) undo-elems)
>             (setq buffer-undo-list new-bul)))))

> and then

>     (defun my-undo-combining (undo-elems)
>       (let ((inhibit-modification-hooks t))
>         (while t
>           (primitive-undo 1 undo-elems))))

I've got a first version of some real working code.  Not to be forgotten
is the need for my-undo-combining (which I've called
wrap-and-run-primitive-undo) itself to generate an (apply ...
my-undo-combining ...) for the use of redo.

Here's that code.  It badly needs doc strings and commenting, and there
are one or two safety checks not yet there.  But it is working code.
What do you think?



(defun wrap-and-run-primitive-undo (beg end list)
  (let ((old-bul buffer-undo-list)
	(end-marker (copy-marker end)))
    (if (not inhibit-modification-hooks)
	(run-hook-with-args 'before-change-functions beg end))
    (let ((inhibit-modification-hooks t))
      (funcall #'primitive-undo 1 list))
    (unless (eq buffer-undo-list t)
      (let ((ap-elt
	     (list 'apply
		   (- end end-marker)
		   beg
		   (marker-position end-marker)
		   #'wrap-and-run-primitive-undo
		   beg (marker-position end-marker) buffer-undo-list))
	    (ptr buffer-undo-list))
	(if (not (eq buffer-undo-list old-bul))
	    (progn
	      (while (not (eq (cdr ptr) old-bul))
		(setq ptr (cdr ptr)))
	      (setcdr ptr nil)
	      (push ap-elt buffer-undo-list)
	      (setcdr buffer-undo-list old-bul)))))
    (if (not inhibit-modification-hooks)
	(run-hook-with-args 'after-change-functions
			    beg (marker-position end-marker)
			    (- end beg)))))
	  
(defmacro combine-change-calls (beg end &rest forms)
  `(let ((-beg- ,beg)
	 (-end- ,end)
	 (body (lambda () ,@forms))
	 (old-bul buffer-undo-list)
	 end-marker)
     (setq end-marker (copy-marker -end-))
     (if (not inhibit-modification-hooks)
	 (run-hook-with-args 'before-change-functions -beg- -end-))
     (if (eq buffer-undo-list t)
	 (progn
	   (funcall body)
	   (if (not inhibit-modification-hooks)
	       (run-hook-with-args 'after-change-functions
				   -beg- (marker-position end-marker)
				   (- -end- -beg-))))
       (let ((inhibit-modification-hooks t))
	 (funcall body))
       (let ((ap-elt
	      (list 'apply
		    (- -end- end-marker) ; DELTA
		    -beg-
		    (marker-position end-marker)
		    #'wrap-and-run-primitive-undo
		    -beg- (marker-position end-marker) buffer-undo-list))
	     (ptr buffer-undo-list))
	 (if (not (eq buffer-undo-list old-bul))
	     (progn
	       (while (not (eq (cdr ptr) old-bul))
		 (setq ptr (cdr ptr)))
	       (setcdr ptr nil)
	       (push ap-elt buffer-undo-list)
	       (setcdr buffer-undo-list old-bul)))))
     (if (not inhibit-modification-hooks)
	 (run-hook-with-args 'after-change-functions
			     -beg- (marker-position end-marker)
			     (- -end- -beg-)))
     (setq end-marker nil)))


As usual, comments and criticism of the above would be welcome.

[ .... ]

> I could imagine a macro `with-inhibit-read-only` which would bind
> inhibit-read-only to t and then tweak the buffer-undo-list similarly to
> what you're doing such that the undo is also performed with
> inhibit-read-only bound to t.

OK, thanks.  The above code hasn't taken this into account

[ .... ]

> I'm not interested in destroying this information.  I'm just trying to
> solve the problem at hand and it's a simple and efficient solution, and
> neither you nor I can come up with a scenario where this simpler
> solution is worse, apparently.

Well, I've wanted to see this info whilst writing c-c-c.

> > Its continued existence does not cause any disadvantage.  The run time
> > which would be saved in `undo' is minimal and inconsequential.

> Agreed, the gain is in the simplicity of combine-change-calls.

I can definitely see that.  ;-)

[ .... ]

> >> IIUC we agree that this is considered an unimportant use-case and it's
> >> OK to just ignore such boundaries.
> > I suppose so.  What would such a boundary mean anyway?  We're talking
> > about a scenario where all the change hook invocations are amalgamated.
> > How could it make sense to split that up?

> Some commands explicitly push undo boundaries in the middle of their
> execution, so that the user can easily undo the 2nd half of the
> command's execution, for example.

> This is rare enough that I think it's perfectly OK to say that such
> use-cases are not supported by combine-change-calls.  [ It's also the
> only use-case I could think of where using a simple pair of
> "delete+insert" undo entries would have made a difference.  ]

To be mulled over.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: An idea: combine-change-calls

[Stefan Monnier]

> But the `(apply ...)' forms are a problem for exactly these other
> things. It's too powerful. The packages that manipulate buffer-undo-list
> can't (and don't try to) handle `apply' properly, since it can contain
> arbitrary effects.

AFAIK the (apply ...) has the needed constraints for undo-in-region to
handle it properly.  If not, we should fix it.
So, AFAIK it's handled properly by undo itself, undo-tree, and
undo-in-region.  Which "package that manipulate buffer-undo-list" are
you thinking of?


        Stefan

Re: An idea: combine-change-calls

[Stefan Monnier]

> (defun wrap-and-run-primitive-undo (beg end list)
>   (let ((old-bul buffer-undo-list)
> 	(end-marker (copy-marker end)))

I'd have expected it to start with:

    (defun wrap-and-run-primitive-undo (beg end list)
      (combine-change-calls

also this is an internal function that will only be useful for
combine-change-calls, so it needs to have a "--" in its name.

>     (let ((inhibit-modification-hooks t))
>       (funcall #'primitive-undo 1 list))

I think you want something like

    (while list
      (setq list (primitive-undo 1 list))

in case there are some undo-boundaries in `list`.

> (defmacro combine-change-calls (beg end &rest forms)
>   `(let ((-beg- ,beg)

It now occurs to me that it's probable preferable to do:

    (defmacro combine-change-calls (beg end &rest forms)
      `(combine-change-calls-function ,beg ,end (lambda () ,@forms)))

and then

    (defun combine-change-calls-function (beg rest body)
      ... do the heavy lifting here ...)

> 	       (while (not (eq (cdr ptr) old-bul))
> 		 (setq ptr (cdr ptr)))

This can inf-loop if the new buffer-undo-list happens not to be an
extension of the old list any more.

>> I could imagine a macro `with-inhibit-read-only` which would bind
>> inhibit-read-only to t and then tweak the buffer-undo-list similarly to
>> what you're doing such that the undo is also performed with
>> inhibit-read-only bound to t.
> OK, thanks.  The above code hasn't taken this into account

That was for the case that you decided not to use (apply ...) but some
new undo-element.


        Stefan

Re: An idea: combine-change-calls

[Alan Mackenzie]

Hello, Stefan.

On Fri, Mar 30, 2018 at 11:05:24 -0400, Stefan Monnier wrote:
> > (defun wrap-and-run-primitive-undo (beg end list)
> >   (let ((old-bul buffer-undo-list)
> > 	(end-marker (copy-marker end)))

> I'd have expected it to start with:

>     (defun wrap-and-run-primitive-undo (beg end list)
>       (combine-change-calls

That's a refinement I haven't managed to get around to, yet.

> also this is an internal function that will only be useful for
> combine-change-calls, so it needs to have a "--" in its name.

DONE.

> >     (let ((inhibit-modification-hooks t))
> >       (funcall #'primitive-undo 1 list))

> I think you want something like

>     (while list
>       (setq list (primitive-undo 1 list))

Indeed so.  (funcall #'...) is stupid.

> in case there are some undo-boundaries in `list`.

> > (defmacro combine-change-calls (beg end &rest forms)
> >   `(let ((-beg- ,beg)

> It now occurs to me that it's probable preferable to do:

>     (defmacro combine-change-calls (beg end &rest forms)
>       `(combine-change-calls-function ,beg ,end (lambda () ,@forms)))

DONE, at least with the name combine-change-calls-1.  I don't really
like "...-function", since that suggests a hook variable.

> and then

>     (defun combine-change-calls-function (beg rest body)
>       ... do the heavy lifting here ...)

DONE.

> > 	       (while (not (eq (cdr ptr) old-bul))
> > 		 (setq ptr (cdr ptr)))

> This can inf-loop if the new buffer-undo-list happens not to be an
> extension of the old list any more.

Not only can, but does.  I spent around 2 hours yesterday waiting for
this infinite loop to terminate.  ;-(  It is, of course, garbage
collection which lops off the element we're looking for, particularly
when commenting out a large region.

[ .... ]

I have also been optimising (or, more accurately, de-pessimizing) CC
Mode to cope better with large blocks of comments.  Together with
combine-change-calls on {,un}comment-region, comment-region now takes
0.4 seconds to comment out Nil Geisweiller's (the OP of bug #30735) 2500
line test file.  Undo takes even less time.  His original complaint was
of a time of 10 minutes.  C-c C-c on xdisp.c takes around 8 seconds
(though I don't recommend you to try this yourself before I've committed
these CC Mode changes.  ;-)

So, I think this exercise is worthwhile.  I have added some doc strings
to the functions.  I have introduced a variable to hinder a "recursive"
undo--wrap-and-run-primitive-undo appearing in the undo list.  This
happened with uncomment-region calling comment-region.

Sometimes, point ends up at the wrong place after an undo (probably
because of primitive-undo removing POSITION elements from undo-lists, as
we've already discussed.)

If you want to try this out yourself, here are the versions of
{,un}comment-region I've been using for testing:

(defun comment-region (beg end &optional arg)
  "Comment or uncomment each line in the region.
With just \\[universal-argument] prefix arg, uncomment each line in region BEG .. END.
Numeric prefix ARG means use ARG comment characters.
If ARG is negative, delete that many comment characters instead.

The strings used as comment starts are built from `comment-start'
and `comment-padding'; the strings used as comment ends are built
from `comment-end' and `comment-padding'.

By default, the `comment-start' markers are inserted at the
current indentation of the region, and comments are terminated on
each line (even for syntaxes in which newline does not end the
comment and blank lines do not get comments).  This can be
changed with `comment-style'."
  (interactive "*r\nP")
  (comment-normalize-vars)
  (if (> beg end) (let (mid) (setq mid beg beg end end mid)))
  (save-excursion
    ;; FIXME: maybe we should call uncomment depending on ARG.
    (combine-change-calls beg end
                          (funcall comment-region-function beg end arg))))

(defun uncomment-region (beg end &optional arg)
  "Uncomment each line in the BEG .. END region.
The numeric prefix ARG can specify a number of chars to remove from the
comment markers."
  (interactive "*r\nP")
  (comment-normalize-vars)
  (when (> beg end) (setq beg (prog1 end (setq end beg))))
  ;; Bind `comment-use-global-state' to nil.  While uncommenting a region
  ;; (which works a line at a time), a comment can appear to be
  ;; included in a mult-line string, but it is actually not.
  (let ((comment-use-global-state nil))
    (save-excursion
      (combine-change-calls beg end
                            (funcall uncomment-region-function beg end arg)))))


And here is the current state of combine-change-calls itself:


(defvar undo--combining-change-calls nil
  "Non-nil when `combine-change-calls' is running.")

(defun undo--wrap-and-run-primitive-undo (beg end list)
  "Call `primitive-undo' on the undo elements in LIST.

This function is intended to be called purely by `undo' as the
function in an \(apply DELTA BEG END FUNNAME . ARGS) undo
element.  It invokes `before-change-functions' and
`after-change-functions' once each for the entire region \(BEG
END) rather than once for each individual change.

Additionally the fresh \"redo\" elements which are generated on
`buffer-undo-list' will themselves be \"enclosed\" in
`undo--wrap-and-run-primitive-undo'.

Undo elements of this form are generated by the macro
`combine-change-calls'."
  (let ((old-bul buffer-undo-list)
	(end-marker (copy-marker end)))
    (if (not inhibit-modification-hooks)
	(run-hook-with-args 'before-change-functions beg end))
    (let ((inhibit-modification-hooks t))
      (while list
	(setq list (primitive-undo 1 list))))
    (unless (eq buffer-undo-list t)
      (let ((ap-elt
	     (list 'apply
		   (- end end-marker)
		   beg
		   (marker-position end-marker)
		   #'undo--wrap-and-run-primitive-undo
		   beg (marker-position end-marker) buffer-undo-list))
	    (ptr buffer-undo-list))
	(if (not (eq buffer-undo-list old-bul))
	    (progn
	      (while (and (cdr ptr)
			  (not (eq (cdr ptr) old-bul)))
		(setq ptr (cdr ptr)))
	      (unless (cdr ptr)
		(message "w-a-r-p-undo: buffer-undo-list broken"))
	      (setcdr ptr nil)
	      (push ap-elt buffer-undo-list)
	      (setcdr buffer-undo-list old-bul)))))
    (if (not inhibit-modification-hooks)
	(run-hook-with-args 'after-change-functions
			    beg (marker-position end-marker)
			    (- end beg)))))
	  
(defun combine-change-calls-1 (beg end body)
  "Evaluate BODY, running the change hooks just once, for region \(BEG END).

Firstly, `before-change-functions' is invoked for the region
\(BEG END), then BODY (a function) is evaluated with
`inhibit-modification-hooks' non-nil, then finally
`after-change-functions' is invoked on the updated region (BEG
NEW-END) with a calculated OLD-LEN argument.  If
`inhibit-modification-hooks' is initially non-nil, the change
hooks are not run.

The result of `comebine-change-calls-1' is the value returned by
BODY.  BODY must not make a different buffer current, except
temporarily.  It must not make any changes to the buffer outside
the specified region.

Additionally, the buffer modifications of BODY are recorded on
the buffer's undo list as a single \(apply ...) entry containing
the function `undo--wrap-and-run-primitive-undo'."
  (let ((old-bul buffer-undo-list)
	(end-marker (copy-marker end)))
    (if undo--combining-change-calls
	(funcall body)
      (let ((undo--combining-change-calls t))
	(if (not inhibit-modification-hooks)
	    (run-hook-with-args 'before-change-functions beg end))
	(prog1
	    (if (eq buffer-undo-list t)
		(progn
		  (funcall body)
		  (if (not inhibit-modification-hooks)
		      (run-hook-with-args 'after-change-functions
					  beg (marker-position end-marker)
					  (- end beg))))
	      (prog1
		  (let ((inhibit-modification-hooks t))
		    (funcall body))
		(let ((ap-elt
		       (list 'apply
			     (- end end-marker)
			     beg
			     (marker-position end-marker)
			     #'undo--wrap-and-run-primitive-undo
			     beg (marker-position end-marker) buffer-undo-list))
		      (ptr buffer-undo-list))
		  (if (not (eq buffer-undo-list old-bul))
		      (progn
			(while (and (cdr ptr)
				    (not (eq (cdr ptr) old-bul)))
			  (setq ptr (cdr ptr)))
			(unless (cdr ptr)
			  (message "combine-change-calls: buffer-undo-list broken"))
			(setcdr ptr nil)
			(push ap-elt buffer-undo-list)
			(setcdr buffer-undo-list old-bul))))))
	  (if (not inhibit-modification-hooks)
	      (run-hook-with-args 'after-change-functions
				  beg (marker-position end-marker)
				  (- end beg)))
	  (setq end-marker nil))))))

(defmacro combine-change-calls (beg end &rest forms)
  "Evaluate FORMS, running the change hooks just once.

Firstly, `before-change-functions' is invoked for the region
\(BEG END), then the FORMS are evaluated with
`inhibit-modification-hooks' non-nil, and finally
`after-change-functions' is invoked on the updated region.  The
change hooks are not run if `inhibit-modification-hooks' is
non-nil.

The result of `combine-change-calls' is the value returned by the
last of the FORMS to be evaluated.  FORMS may not make a
different buffer current, except temporarily.  FORMS may not
change the buffer outside the specified region.

Additionally, the buffer modifications of BODY are recorded on
the buffer's undo list as a single \(apply ...) entry containing
the function `undo--wrap-and-run-primitive-undo'. "
  `(combine-change-calls-1 ,beg ,end (lambda () ,@forms)))



>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: An idea: combine-change-calls

[Stefan Monnier]

> >     (defun wrap-and-run-primitive-undo (beg end list)
> >       (combine-change-calls
> That's a refinement I haven't managed to get around to, yet.

Any difficulty there?
Naively, I'd expect that it's just a matter of:

    (defun undo--wrap-and-run-primitive-undo (beg end list)
      (combine-change-calls
        (while list
          (setq list (primitive-undo 1 list)))))
  
[ and of course, making sure this function is defined after the
  combine-change-calls macro.  ]

>>     (defmacro combine-change-calls (beg end &rest forms)
>>       `(combine-change-calls-function ,beg ,end (lambda () ,@forms)))
> DONE, at least with the name combine-change-calls-1.

Good.

> I don't really like "...-function", since that suggests
> a hook variable.

I'm not too fond of those either for the same reason, although I must
admit to using such names occasionally.

> Not only can, but does.  I spent around 2 hours yesterday waiting for
> this infinite loop to terminate.  ;-(

I'm sure it was just a question of time.

> It is, of course, garbage collection which lops off the element we're
> looking for, particularly when commenting out a large region.

Yes, that should be the most common reason.

> So, I think this exercise is worthwhile.  I have added some doc strings
> to the functions.  I have introduced a variable to hinder a "recursive"
> undo--wrap-and-run-primitive-undo appearing in the undo list.  This
> happened with uncomment-region calling comment-region.

That's weird: shouldn't the inhibit-modification-hooks already play this role?

Oh, wait, I see you don't test inhibit-modification-hooks any more but
you test undo--combining-change-calls instead.  Any particular reason
for this change?

> Sometimes, point ends up at the wrong place after an undo (probably
> because of primitive-undo removing POSITION elements from undo-lists, as
> we've already discussed.)

The code that removes those "position" elements is in `undo`, not in
`primitive-undo`, so what you're describing must come from something
else (unless you're seeing this when *redoing* or *reundoing*).


        Stefan

Re: An idea: combine-change-calls

[Alan Mackenzie]

Hello, Stefan.

On Sat, Mar 31, 2018 at 19:38:03 -0400, Stefan Monnier wrote:
> > >     (defun wrap-and-run-primitive-undo (beg end list)
> > >       (combine-change-calls
> > That's a refinement I haven't managed to get around to, yet.

> Any difficulty there?
> Naively, I'd expect that it's just a matter of:

>     (defun undo--wrap-and-run-primitive-undo (beg end list)
>       (combine-change-calls
>         (while list
>           (setq list (primitive-undo 1 list)))))
  
> [ and of course, making sure this function is defined after the
>   combine-change-calls macro.  ]

It was exactly that.  No difficulties.

[ .... ]

> > So, I think this exercise is worthwhile.  I have added some doc strings
> > to the functions.  I have introduced a variable to hinder a "recursive"
> > undo--wrap-and-run-primitive-undo appearing in the undo list.  This
> > happened with uncomment-region calling comment-region.

> That's weird: shouldn't the inhibit-modification-hooks already play this role?

I don't think so.  i-m-h is purely to do with whether the hooks should
be run now.  That has nothing to do with how items should be added to
the undo list.

> Oh, wait, I see you don't test inhibit-modification-hooks any more but
> you test undo--combining-change-calls instead.  Any particular reason
> for this change?

inhibit-modification-hooks is tested around the run-hooks-with-args
calls.  If i-m-h is set to non-nil by some function, and comment-region
is run, we still want the undo records to be enclosed by an (apply ...)
form.

> > Sometimes, point ends up at the wrong place after an undo (probably
> > because of primitive-undo removing POSITION elements from undo-lists, as
> > we've already discussed.)

> The code that removes those "position" elements is in `undo`, not in
> `primitive-undo`, so what you're describing must come from something
> else (unless you're seeing this when *redoing* or *reundoing*).

I'm not sure.  I think it's in re-undoing.  As you've already guessed, I
would be in favour of removing that code from `undo' that we don't see
what it's for.

Anyhow, here's another version of the code (below), with the following
changes:
1/- undo--w-a-r-p-undo is now a macro call of c-c-c.
2/- end-marker is now of type t, so that it is relocated when text is
inserted at the postion it points to.  (DELTA was coming out wrong.)
3/- end-marker is now set to point nowhere at the end of the function.
4/- A whole lot of `prog1's have been replaced by the variable
RESULT.
5/- c-c-c-1 no longer incorporates a timestamp undo record ((t . ...))
into the (apply ...) form.  This just seemed wrong.

I think it's almost time to commit this to master.  I propose that
combine-change-calls{-1,} go into subr.el, beside
combine-after-change-calls, and undo--wrap-and-run-primitive-undo go
into simple.el, beside the rest of the undo stuff.

I will introduce the variable comment-combine-change-calls in
newcomment.el, and modify {,un}comment-region to test it, and if set, to
use c-c-c.

I will document c-c-c in the Elisp manual on page "Change Hooks".

For (almost) the last time, here's the current version of the code:



(defvar undo--combining-change-calls nil
  "Non-nil when `combine-change-calls' is running.")

(defun combine-change-calls-1 (beg end body)
  "Evaluate BODY, running the change hooks just once, for region \(BEG END).

Firstly, `before-change-functions' is invoked for the region
\(BEG END), then BODY (a function) is evaluated with
`inhibit-modification-hooks' non-nil, then finally
`after-change-functions' is invoked on the updated region (BEG
NEW-END) with a calculated OLD-LEN argument.  If
`inhibit-modification-hooks' is initially non-nil, the change
hooks are not run.

The result of `comebine-change-calls-1' is the value returned by
BODY.  BODY must not make a different buffer current, except
temporarily.  It must not make any changes to the buffer outside
the specified region.  It must not change
`before-change-functions' or `after-change-functions'.

Additionally, the buffer modifications of BODY are recorded on
the buffer's undo list as a single \(apply ...) entry containing
the function `undo--wrap-and-run-primitive-undo'."
  (let ((old-bul buffer-undo-list)
	(end-marker (copy-marker end t))
	result)
    (if undo--combining-change-calls
	(setq result (funcall body))
      (let ((undo--combining-change-calls t))
	(if (not inhibit-modification-hooks)
	    (run-hook-with-args 'before-change-functions beg end))
	(if (eq buffer-undo-list t)
	    (setq result (funcall body))
	  (let ((inhibit-modification-hooks t))
	    (setq result (funcall body)))
	  (let ((ap-elt
		 (list 'apply
		       (- end end-marker)
		       beg
		       (marker-position end-marker)
		       #'undo--wrap-and-run-primitive-undo
		       beg (marker-position end-marker) buffer-undo-list))
		(ptr buffer-undo-list))
	    (if (not (eq buffer-undo-list old-bul))
		(progn
		  (while (and (not (eq (cdr ptr) old-bul))
			      ;; In case garbage collection has removed OLD-BUL.
			      (cdr ptr)
			      ;; Don't include a timestamp entry.
			      (not (and (consp (cdr ptr))
					(consp (cadr ptr))
					(eq (caadr ptr) t)
					(setq old-bul (cdr ptr)))))
		    (setq ptr (cdr ptr)))
		  (unless (cdr ptr)
		    (message "combine-change-calls: buffer-undo-list broken"))
		  (setcdr ptr nil)
		  (push ap-elt buffer-undo-list)
		  (setcdr buffer-undo-list old-bul)))))
	(if (not inhibit-modification-hooks)
	    (run-hook-with-args 'after-change-functions
				beg (marker-position end-marker)
				(- end beg)))))
    (set-marker end-marker nil)
    result))

(defmacro combine-change-calls (beg end &rest forms)
  "Evaluate FORMS, running the change hooks just once.

Firstly, `before-change-functions' is invoked for the region
\(BEG END), then the FORMS are evaluated with
`inhibit-modification-hooks' non-nil, and finally
`after-change-functions' is invoked on the updated region.  The
change hooks are not run if `inhibit-modification-hooks' is
non-nil.

The result of `combine-change-calls' is the value returned by the
last of the FORMS to be evaluated.  FORMS may not make a
different buffer current, except temporarily.  FORMS may not
change the buffer outside the specified region.  It must not
change `before-change-functions' or `after-change-functions'.

Additionally, the buffer modifications of BODY are recorded on
the buffer's undo list as a single \(apply ...) entry containing
the function `undo--wrap-and-run-primitive-undo'. "
  `(combine-change-calls-1 ,beg ,end (lambda () ,@forms)))

(defun undo--wrap-and-run-primitive-undo (beg end list)
  "Call `primitive-undo' on the undo elements in LIST.

This function is intended to be called purely by `undo' as the
function in an \(apply DELTA BEG END FUNNAME . ARGS) undo
element.  It invokes `before-change-functions' and
`after-change-functions' once each for the entire region \(BEG
END) rather than once for each individual change.

Additionally the fresh \"redo\" elements which are generated on
`buffer-undo-list' will themselves be \"enclosed\" in
`undo--wrap-and-run-primitive-undo'.

Undo elements of this form are generated by the macro
`combine-change-calls'."
  (combine-change-calls beg end
			(while list
			  (setq list (primitive-undo 1 list)))))


>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: An idea: combine-change-calls

[Stefan Monnier]

>> That's weird: shouldn't the inhibit-modification-hooks already play this role?
> I don't think so.  i-m-h is purely to do with whether the hooks should
> be run now.  That has nothing to do with how items should be added to
> the undo list.

Ah, I see what you mean.
[ tho I think in practice, there'll be no difference:
  the code which binds inhibit-modification-hooks in the context will
  already want to deal with the fact that the undo entries would normally
  not be run within such a inhibit-modification-hooks, so it'll usually
  bind buffer-undo-list to t or do something related.  ]

> I'm not sure.  I think it's in re-undoing.

Yes, that would be expected, indeed.

> As you've already guessed, I would be in favour of removing that code
> from `undo' that we don't see what it's for.

As I said, it's orthogonal to the current discussion (it affects many
more cases).  And I personally have no opinion on whether this behavior
should be considered as a bug or a feature.

> I think it's almost time to commit this to master.

Agreed.

> I propose that combine-change-calls{-1,} go into subr.el, beside
> combine-after-change-calls, and undo--wrap-and-run-primitive-undo go
> into simple.el, beside the rest of the undo stuff.

I don't have an opinion on which file it should go to, but
undo--wrap-and-run-primitive-undo belongs with the rest, so it'd make
a lot more sense to keep them all three together.

> I will introduce the variable comment-combine-change-calls in
> newcomment.el, and modify {,un}comment-region to test it, and if set, to
> use c-c-c.

I'm not sure we even need such a variable (tho I'd put the
combine-change-calls around comment-region-default, so it doesn't
affect other settings of comment-region-function).

> I will document c-c-c in the Elisp manual on page "Change Hooks".

Thanks.


        Stefan

Re: An idea: combine-change-calls

[Alan Mackenzie]

Hello, Stefan.

On Fri, Mar 30, 2018 at 09:04:06 -0400, Stefan Monnier wrote:
> > But the `(apply ...)' forms are a problem for exactly these other
> > things. It's too powerful. The packages that manipulate buffer-undo-list
> > can't (and don't try to) handle `apply' properly, since it can contain
> > arbitrary effects.

> AFAIK the (apply ...) has the needed constraints for undo-in-region to
> handle it properly.  If not, we should fix it.

Indeed we should.  undo-elt-in-region "Determine whether UNDO-ELT falls
inside the region START ... END." lacks a cond arm for (apply ...), so
always returns nil for it, which means that undo doesn't work at all for
an (apply ...) element when transient-mark-mode is enabled and the mark
is "active" (which is quite common).

> So, AFAIK it's handled properly by undo itself, undo-tree, and
> undo-in-region.  Which "package that manipulate buffer-undo-list" are
> you thinking of?

Actually, undo-in-region is problematic.  Aside from undo-elt-in-region
(see above), which is easy to fix, there is undo-adjust-elt "Return
adjustment of undo element ELT by the undo DELTAS list.", which can't
adjust the contents of an (apply ...) element, since it doesn't know the
internal structure of each type of (apply ...).  In this sense, (apply
...) is indeed too powerful.

Fixing this in general would need something like a property on the
(apply ...)'s FUN-NAME symbol, whose value would be a function for
"returning the adjustment of the matching (apply ...) element".  But
that is getting crazy.

Why do we have undo-in-region at all?  It fouls up the simplicity of the
undo mechanism.  It also seems like a nuisance rather than a feature:
If I were using transient-mark-mode, made some buffer changes, and
somehow had a small "active" region; if I then start undoing the
changes, I would be annoyed if C-x u missed out some undos because they
weren't inside that region.  I think it would make undo unusable.

How about phasing out undo-in-region?  We make it optional in Emacs 27.1
(with the default being off), see if anybody complains, then remove it
entirely in Emacs 28.  undo-in-region is incompatible with (apply ...).
I would say that (apply ...) is the more important of the two.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: An idea: combine-change-calls

[Johan Bockgård]

Alan Mackenzie <acm@muc.de> writes:

> On Fri, Mar 30, 2018 at 09:04:06 -0400, Stefan Monnier wrote:
>
>> So, AFAIK it's handled properly by undo itself, undo-tree, and
>> undo-in-region.  Which "package that manipulate buffer-undo-list" are
>> you thinking of?
>
> Actually, undo-in-region is problematic.  Aside from undo-elt-in-region
> (see above), which is easy to fix, there is undo-adjust-elt "Return
> adjustment of undo element ELT by the undo DELTAS list.", which can't
> adjust the contents of an (apply ...) element, since it doesn't know the
> internal structure of each type of (apply ...).  In this sense, (apply
> ...) is indeed too powerful.

Yes. Another example is ERC (see erc-update-undo-list). Since only the
user's current input at the prompt should be undo-able, undo recording
is enabled for user edits but suppressed for process output (which can
happen concurrently). All positions in buffer-undo-list need to be
adjusted when process output moves the prompt.

Re: An idea: combine-change-calls

[Stefan Monnier]

>> So, AFAIK it's handled properly by undo itself, undo-tree, and
>> undo-in-region.  Which "package that manipulate buffer-undo-list" are
>> you thinking of?
> Actually, undo-in-region is problematic.  Aside from undo-elt-in-region
> (see above), which is easy to fix, there is undo-adjust-elt "Return
> adjustment of undo element ELT by the undo DELTAS list.", which can't
> adjust the contents of an (apply ...) element,

Indeed, that's a very real problem.
I've moved this to bug#31035

> Why do we have undo-in-region at all?

Because it's a great feature.


        Stefan

Re: An idea: combine-change-calls

[Clément Pit-Claudel]

On 2018-04-02 12:25, Alan Mackenzie wrote:
> Why do we have undo-in-region at all?  It fouls up the simplicity of the
> undo mechanism.  It also seems like a nuisance rather than a feature

It's one of the two out-of-the-box killer Emacs feature that I show people :) (that, and C-y M-y to cycle through the kill ring).

Clément.

Re: An idea: combine-change-calls

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

undo-in-region is a very useful feature, on occasion.
Don't delete it!

If it has a bug, fix the bug.


-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.