bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Allen Li]

Interactive commands that act on the region are handled specially such
that when repeated with `repeat-complex-command`, the repeated command
uses the current region rather than the region used for the previous
invocation of the command.

`replace-string` does not respect this; it uses the previous region when
repeated with `repeat-complex-command`.

Note that loading `replace-string` from source (rather than byte
compiled) fixes this problem.  So it's probably a problem with byte
compiled commands.

I swear I filed a bug for this a long time ago, and I can't remember if
it's a regression or it hasn't landed in a release yet.  I can't find
the original bug.

In GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.22, cairo version 1.17.3)
 of 2020-08-28 built on juergen
Windowing system distributor 'The X.Org Foundation', version 11.0.12010000
System Description: Arch Linux

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD JSON
PDUMPER LCMS2 GMP

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Allen Li <darkfeline@felesatra.moe> writes:

> Interactive commands that act on the region are handled specially such
> that when repeated with `repeat-complex-command`, the repeated command
> uses the current region rather than the region used for the previous
> invocation of the command.
>
> `replace-string` does not respect this; it uses the previous region when
> repeated with `repeat-complex-command`.
>
> Note that loading `replace-string` from source (rather than byte
> compiled) fixes this problem.  So it's probably a problem with byte
> compiled commands.

(I'm going through old bug reports that unfortunately weren't resolved
at the time.)

I can reproduce this problem in Emacs 29?

In any case, it's because `replace-string' specifies the start/end
position in the `interactive' spec (as it should), so it lands in
`command-history', and `repeat-complex-command' just executes that.
Other commands, like `flush-lines', have pass in nil as start/end, and
then computes the start/end in the body of the function.

So this can be fixed by rewriting `replace-string' to do the same...
but surely there's a lot of commands out there that say:

  (interactive
[...]
     (list 
	   (if (use-region-p) (region-beginning))

And all of these would have the same problem.  (interactive "r") does
not, because in that case:

(defun foo (start end)
  (interactive "r")
  (message "%s %s" start end))

The following ends up there in the history:

 (foo (region-beginning) (region-end))

Does anybody know of a more general solution to this?

The reason replace-string works when it's not compiled is the because
then this ends up in command-history:

(replace-string "buffer" "foo" nil (if (use-region-p) (region-beginning)) (if (use-region-p) (region-end)) nil nil)

For some reason.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Juri Linkov]

> Does anybody know of a more general solution to this?

This feature is broken by design as I explained in
https://debbugs.gnu.org/45617#17

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Eli Zaretskii]

> Cc: 45607@debbugs.gnu.org, Allen Li <darkfeline@felesatra.moe>
> From: Juri Linkov <juri@linkov.net>
> Date: Tue, 07 Jun 2022 21:40:58 +0300
> 
> > Does anybody know of a more general solution to this?
> 
> This feature is broken by design as I explained in
> https://debbugs.gnu.org/45617#17

It isn't broken, you just expect it to do some magic that it never
meant to do.

As in many other cases, the perfect is the enemy of the good here.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Allen Li]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

On Tue, Jun 7, 2022 at 11:58 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > Cc: 45607@debbugs.gnu.org, Allen Li <darkfeline@felesatra.moe>
> > From: Juri Linkov <juri@linkov.net>
> > Date: Tue, 07 Jun 2022 21:40:58 +0300
> >
> > > Does anybody know of a more general solution to this?
> >
> > This feature is broken by design as I explained in
> > https://debbugs.gnu.org/45617#17
>
> It isn't broken, you just expect it to do some magic that it never
> meant to do.
>
> As in many other cases, the perfect is the enemy of the good here.
>

I think it's reasonable to consider the different behavior of evaled vs
compiled to be a bug.  Which one is correct can be debated, but the fact
that they're different is a bug.  Would you disagree?

[-- Attachment #2: Type: text/html, Size: 1277 bytes --]

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Eli Zaretskii]

> From: Allen Li <darkfeline@felesatra.moe>
> Date: Thu, 9 Jun 2022 01:39:01 -0700
> Cc: Juri Linkov <juri@linkov.net>, Lars Ingebrigtsen <larsi@gnus.org>, 45607@debbugs.gnu.org
> 
> 
> [1:text/plain Show]
> 
> 
> [2:text/html Hide Save:noname (944B)]
> 
> On Tue, Jun 7, 2022 at 11:58 AM Eli Zaretskii <eliz@gnu.org> wrote:
> 
>  > Cc: 45607@debbugs.gnu.org, Allen Li <darkfeline@felesatra.moe>
>  > From: Juri Linkov <juri@linkov.net>
>  > Date: Tue, 07 Jun 2022 21:40:58 +0300
>  > 
>  > > Does anybody know of a more general solution to this?
>  > 
>  > This feature is broken by design as I explained in
>  > https://debbugs.gnu.org/45617#17
> 
>  It isn't broken, you just expect it to do some magic that it never
>  meant to do.
> 
>  As in many other cases, the perfect is the enemy of the good here.
> 
> I think it's reasonable to consider the different behavior of evaled vs compiled to be a bug.  Which one is
> correct can be debated, but the fact that they're different is a bug.  Would you disagree? 

That's not what I alluded to, not at all.  I was talking about
repeat-complex-command itself and its alleged "broken" state.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

>> Does anybody know of a more general solution to this?
>
> This feature is broken by design as I explained in
> https://debbugs.gnu.org/45617#17

Hm...  Is there any way forward here?  Could we institute some special
form to be used in interactive specs that will be recorded in
command-history in a useful manner?  That is, code that today is:

	   (if (use-region-p) (region-beginning))
	   (if (use-region-p) (region-end))

could be something like

          (interactive-region-beginning)
          (interactive-region-end)

and whatever updates command-history would reify those as is instead of
their return values?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> could be something like
>
>           (interactive-region-beginning)
>           (interactive-region-end)
>
> and whatever updates command-history would reify those as is instead of
> their return values?

Sometimes it's helpful to actually look at the code.  All this magic
comes from:

/* If the list of args INPUT was produced with an explicit call to
   `list', look for elements that were computed with
   (region-beginning) or (region-end), and put those expressions into
   VALUES instead of the present values.

   This function doesn't return a value because it modifies elements
   of VALUES to do its job.  */

static void
fix_command (Lisp_Object input, Lisp_Object values)
{
  /* FIXME: Instead of this ugly hack, we should provide a way for an
     interactive spec to return an expression/function that will re-build the
     args without user intervention.  */
  if (CONSP (input))


And what this does is to try to hack its way through the lisp code in an
interactive spec like

      /* Skip through certain special forms.  */
      while (EQ (car, Qlet) || EQ (car, Qletx)
	     || EQ (car, Qsave_excursion)
	     || EQ (car, Qprogn))

looking for `region-beginning' and friends.  But we now byte-compile the
interactive specs, so all this fails spectacularly.

So we need a brand new way to specify which options are
`region-beginning' etc.  Perhaps with a declare form?  (That translates
into symbol properties, I guess.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> So we need a brand new way to specify which options are
> `region-beginning' etc.  Perhaps with a declare form?  (That translates
> into symbol properties, I guess.)

I.e.,

(defun replace-string (from-string to-string &optional delimited start end backward region-noncontiguous-p)
...
  (declare (arg start (if (use-region-p) (region-beginning)))
           (arg end (if (use-region-p) (region-end))))

and fix_command would pick them up from the symbol plist and use those
forms instead of the value for these arguments.

This could be generally useful if we have other things like this that we
want to have reified in a particular way in the command history.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Drew Adams]

I thank you for trying to fix this.  And
what you've said so far make sense to me.
This problem has long bugged me.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I.e.,
>
> (defun replace-string (from-string to-string &optional delimited start end backward region-noncontiguous-p)
> ...
>   (declare (arg start (if (use-region-p) (region-beginning)))
>            (arg end (if (use-region-p) (region-end))))
>
> and fix_command would pick them up from the symbol plist and use those
> forms instead of the value for these arguments.

If we do that, it would be impossible to explicitly specify START and
END values that are different from an active region from ELisp code. If
the region is active, those arguments would always just be ignored.

We would substitute one ugly corner case with another one, but would
have added more semantic complexity.

We only have a problem for `repeat-complex-command' usage, right?  Then
the effect of a new `declare' spec should better be limited to the value
added to `command-history'.

Michael.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> (defun replace-string (from-string to-string &optional delimited start end backward region-noncontiguous-p)
>> ...
>>   (declare (arg start (if (use-region-p) (region-beginning)))
>>            (arg end (if (use-region-p) (region-end))))
>>
>> and fix_command would pick them up from the symbol plist and use those
>> forms instead of the value for these arguments.

[...]

> We only have a problem for `repeat-complex-command' usage, right?  Then
> the effect of a new `declare' spec should better be limited to the value
> added to `command-history'.

Isn't that all that fix_command does?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> > We only have a problem for `repeat-complex-command' usage, right?  Then
> > the effect of a new `declare' spec should better be limited to the value
> > added to `command-history'.
>
> Isn't that all that fix_command does?

Please ignore my comment if it does, I haven't checked.

Michael.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Juri Linkov]

>> (defun replace-string (from-string to-string &optional delimited start end backward region-noncontiguous-p)
>> ...
>>   (declare (arg start (if (use-region-p) (region-beginning)))
>>            (arg end (if (use-region-p) (region-end))))
>>
>> and fix_command would pick them up from the symbol plist and use those
>> forms instead of the value for these arguments.
>
> If we do that, it would be impossible to explicitly specify START and
> END values that are different from an active region from ELisp code. If
> the region is active, those arguments would always just be ignored.

Indeed, some users might want to have numbers for START and END values
to repeat the command exactly on the same previous region, but other users
might want to repeat the command on a newly selected region with
(region-beginning)/(region-end) in the command history.

OTOH, keeping numbers in the history breaks replace-string for
rectangular regions, because the command history will contain
inconsistent numbers: some numbers from the previous replacement,
and other numbers from the new rectangular region.  For example:

  (replace-string "buffer" "foo" nil 1 2 nil '((3 . 4) (5 . 6)))

where 1 2 are the old region boundaries, and '((3 . 4) (5 . 6))
is a new rectangular region boundaries.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

>>> (defun replace-string (from-string to-string &optional delimited start end backward region-noncontiguous-p)
>>> ...
>>>   (declare (arg start (if (use-region-p) (region-beginning)))
>>>            (arg end (if (use-region-p) (region-end))))

[...]

> Indeed, some users might want to have numbers for START and END values
> to repeat the command exactly on the same previous region, but other users
> might want to repeat the command on a newly selected region with
> (region-beginning)/(region-end) in the command history.

Have a look at fix_command -- it tries to parse code in an interactive
spec to find instances of

  preserved_fns = pure_list (intern_c_string ("region-beginning"),
			     intern_c_string ("region-end"),
			     intern_c_string ("point"),
			     intern_c_string ("mark"));

in the code.  (Which doesn't work now, of course, since the spec is
byte-compiled.)  My `declare' suggestion would just make this work
again, and fix a regression.  That is, this isn't new functionality.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Juri Linkov]

>>>> (defun replace-string (from-string to-string &optional delimited start end backward region-noncontiguous-p)
>>>> ...
>>>>   (declare (arg start (if (use-region-p) (region-beginning)))
>>>>            (arg end (if (use-region-p) (region-end))))
>
> My `declare' suggestion would just make this work again, and fix
> a regression.  That is, this isn't new functionality.

Does `declare' put some property on the command's symbol?
Then if a user doesn't want this fix_command thing, it's
easy to customize and remove a special property from the symbol
of a command like `replace-string'.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> Does `declare' put some property on the command's symbol?
> Then if a user doesn't want this fix_command thing, it's
> easy to customize and remove a special property from the symbol
> of a command like `replace-string'.

`declare' forms can do basically anything, but, yes, in this case, I
think a symbol property would make the most sense.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> `declare' forms can do basically anything, but, yes, in this case, I
> think a symbol property would make the most sense.

I've now fixed the replace-string problem in Emacs 29.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Juri Linkov]

>> `declare' forms can do basically anything, but, yes, in this case, I
>> think a symbol property would make the most sense.
>
> I've now fixed the replace-string problem in Emacs 29.

Should the same interactive-args now be added to other
replacement commands?

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

>> I've now fixed the replace-string problem in Emacs 29.
>
> Should the same interactive-args now be added to other
> replacement commands?

It should be added to all commands that work on the region like this,
yes.  But I wondered whether we should make some trivial helper
functions first like

(defun use-region-beginning ()
  "Return the start of the region if `use-region-p'."
  (and (use-region-p) (region-beginning)))

and the same for -end to avoid having to repeat that code phrase so many
places.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Juri Linkov]

> It should be added to all commands that work on the region like this,
> yes.  But I wondered whether we should make some trivial helper
> functions first like
>
> (defun use-region-beginning ()
>   "Return the start of the region if `use-region-p'."
>   (and (use-region-p) (region-beginning)))
>
> and the same for -end to avoid having to repeat that code phrase so many
> places.

Indeed, this will help to make the history items shorter:

  (replace-string "a" "b" nil (if (use-region-p) (region-beginning)) (if (use-region-p) (region-end)))
  ->
  (replace-string "a" "b" nil (use-region-beginning) (use-region-end))

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Eli Zaretskii]

> Cc: Michael Heerdegen <michael_heerdegen@web.de>, 45607@debbugs.gnu.org,
>  Allen Li <darkfeline@felesatra.moe>
> From: Juri Linkov <juri@linkov.net>
> Date: Tue, 09 Aug 2022 21:41:24 +0300
> 
> > It should be added to all commands that work on the region like this,
> > yes.  But I wondered whether we should make some trivial helper
> > functions first like
> >
> > (defun use-region-beginning ()
> >   "Return the start of the region if `use-region-p'."
> >   (and (use-region-p) (region-beginning)))
> >
> > and the same for -end to avoid having to repeat that code phrase so many
> > places.
> 
> Indeed, this will help to make the history items shorter:
> 
>   (replace-string "a" "b" nil (if (use-region-p) (region-beginning)) (if (use-region-p) (region-end)))
>   ->
>   (replace-string "a" "b" nil (use-region-beginning) (use-region-end))

Bonus points for calling use-region-p just once, not twice.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Bonus points for calling use-region-p just once, not twice.

I don't really see a convenient way to do that in these cases -- the
only ones I could think of (adding a new function to return both
start/end, and then splice the results in into the `interactive' specs
etc) would lead to obfuscated code.

(And it's not like this is performance critical anyway.)

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Juri Linkov <juri@linkov.net>,  michael_heerdegen@web.de,
>   45607@debbugs.gnu.org,  darkfeline@felesatra.moe
> Date: Tue, 09 Aug 2022 21:15:51 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Bonus points for calling use-region-p just once, not twice.
> 
> I don't really see a convenient way to do that in these cases -- the
> only ones I could think of (adding a new function to return both
> start/end, and then splice the results in into the `interactive' specs
> etc) would lead to obfuscated code.

What about cl-destructuring-bind and its ilk?

> (And it's not like this is performance critical anyway.)

It's IMO inelegant to make the same test twice in a row.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> What about cl-destructuring-bind and its ilk?

That'd possible, of course, but awkward.

>> (And it's not like this is performance critical anyway.)
>
> It's IMO inelegant to make the same test twice in a row.

Yup.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> Indeed, this will help to make the history items shorter:
>
>   (replace-string "a" "b" nil (if (use-region-p) (region-beginning))
> (if (use-region-p) (region-end)))
>   ->
>   (replace-string "a" "b" nil (use-region-beginning) (use-region-end))

OK, so I've now done this.  So somebody™ should go through the code base
and adjust the code and add the new `declare' forms.  😀

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Juri Linkov]

>>   (replace-string "a" "b" nil (if (use-region-p) (region-beginning))
>> (if (use-region-p) (region-end)))
>>   ->
>>   (replace-string "a" "b" nil (use-region-beginning) (use-region-end))
>
> OK, so I've now done this.  So somebody™ should go through the code base
> and adjust the code and add the new `declare' forms.  😀

Interesting, there are not too many uses of this pattern,
and most of them are related to replacement commands.
Ok, I could replace them with adding interactive-args.

lisp/isearch.el
2392:      (if (use-region-p) (region-beginning))
2393:      (if (use-region-p) (region-end))
lisp/replace.el
464: 	   (if (use-region-p) (region-beginning))
465: 	   (if (use-region-p) (region-end))
558: 	   (if (use-region-p) (region-beginning))
559: 	   (if (use-region-p) (region-end))
606: 	   (if (use-region-p) (region-beginning))
607: 	   (if (use-region-p) (region-end))
761: 	   (if (use-region-p) (region-beginning))
762: 	   (if (use-region-p) (region-end))
lisp/textmodes/paragraphs.el
518:                      (if (use-region-p) (region-beginning))
519:                      (if (use-region-p) (region-end))))
lisp/vc/log-view.el
581:    (list (if (use-region-p) (region-beginning) (point))
582:          (if (use-region-p) (region-end) (point))))
596:    (list (if (use-region-p) (region-beginning) (point))
597:          (if (use-region-p) (region-end) (point))))

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> Interesting, there are not too many uses of this pattern,
> and most of them are related to replacement commands.
> Ok, I could replace them with adding interactive-args.
>
> lisp/isearch.el
> 2392:      (if (use-region-p) (region-beginning))
> 2393:      (if (use-region-p) (region-end))
> lisp/replace.el
> 464: 	   (if (use-region-p) (region-beginning))
> 465: 	   (if (use-region-p) (region-end))
> 558: 	   (if (use-region-p) (region-beginning))
> 559: 	   (if (use-region-p) (region-end))
> 606: 	   (if (use-region-p) (region-beginning))
> 607: 	   (if (use-region-p) (region-end))
> 761: 	   (if (use-region-p) (region-beginning))
> 762: 	   (if (use-region-p) (region-end))
> lisp/textmodes/paragraphs.el
> 518:                      (if (use-region-p) (region-beginning))
> 519:                      (if (use-region-p) (region-end))))
> lisp/vc/log-view.el
> 581:    (list (if (use-region-p) (region-beginning) (point))
> 582:          (if (use-region-p) (region-end) (point))))
> 596:    (list (if (use-region-p) (region-beginning) (point))
> 597:          (if (use-region-p) (region-end) (point))))

That's fewer than I'd have guessed -- but I guess that quite a few
commands stash the logic down into the function body instead of putting
it into the `interactive' spec.  Commands like `duplicate-dwim', for
instance, could be pretty easily fixed in that way, for instance.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Juri Linkov]

>> 2392:      (if (use-region-p) (region-beginning))
>> 2393:      (if (use-region-p) (region-end))
>
> That's fewer than I'd have guessed -- but I guess that quite a few
> commands stash the logic down into the function body instead of putting
> it into the `interactive' spec.  Commands like `duplicate-dwim', for
> instance, could be pretty easily fixed in that way, for instance.

OTOH, `duplicate-dwim' is optimized to call `use-region-p' only once:

  (cond
   ((use-region-p)
    (let* ((beg (region-beginning))
           (end (region-end))

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> OTOH, `duplicate-dwim' is optimized to call `use-region-p' only once:
>
>   (cond
>    ((use-region-p)
>     (let* ((beg (region-beginning))
>            (end (region-end))

I don't think that makes much difference when it comes to interactive
specs:

(benchmark-run 1000000 (use-region-p))
=> (0.038645288 0 0.0)

If you're calling `duplicate-dwim' interactively more than a million
times a second, you'll get a slowdown of a couple hundredths of a
second.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Juri Linkov]

>> OTOH, `duplicate-dwim' is optimized to call `use-region-p' only once:
>>
>>   (cond
>>    ((use-region-p)
>>     (let* ((beg (region-beginning))
>>            (end (region-end))
>
> I don't think that makes much difference when it comes to interactive
> specs:
>
> (benchmark-run 1000000 (use-region-p))
> => (0.038645288 0 0.0)
>
> If you're calling `duplicate-dwim' interactively more than a million
> times a second, you'll get a slowdown of a couple hundredths of a
> second.

But still `use-region-p' is used to start a new logical branch of `cond'.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> But still `use-region-p' is used to start a new logical branch of `cond'.

The suggestion was to put that logic into the interactive spec and bind
to new parameters beg/end (like other commands do).  The `cond' would
then be adjusted to react to beg/end.

bug#45607: 27.1; compiled replace-string breaks repeat-complex-command

[Juri Linkov]

>>>   (replace-string "a" "b" nil (if (use-region-p) (region-beginning))
>>> (if (use-region-p) (region-end)))
>>>   ->
>>>   (replace-string "a" "b" nil (use-region-beginning) (use-region-end))
>>
>> OK, so I've now done this.  So somebody™ should go through the code base
>> and adjust the code and add the new `declare' forms.  😀
>
> Interesting, there are not too many uses of this pattern,
> and most of them are related to replacement commands.
> Ok, I could replace them with adding interactive-args.

Done, with adding also 'use-region-noncontiguous-p'.