Package: emacs;
Reported by: Stefan Monnier <monnier <at> iro.umontreal.ca>
Date: Sat, 15 Feb 2025 22:19:02 UTC
Severity: normal
Tags: patch
View this message in rfc822 format
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 76313 <at> debbugs.gnu.org, Stefan Kangas <stefankangas <at> gmail.com>, tsdh <at> gnu.org Subject: bug#76313: New function `replace-region` Date: Sun, 30 Mar 2025 00:15:54 -0400
[Message part 1 (text/plain, inline)]
>> That would be the best way to do it, if the normal case is that you want
>> the costly algorithm. But my understanding so far is that, in the
>> overwhelming majority of cases, you do not want this.
> I don't think this assumption is true. The slow and costly algorithm
> keeps the markers and other properties, whereas bypassing it will not
> do so. So the trade-off is not clear and depends on the context.
My guts agree with Stefan, but I think none of us have hard data to
confirm our intuition.
This said, I did a quick:
grep -C1 delete-region **/*.el |
sed 's/-\([0-9]\+\)-/:\1:/' |
grep insert
and this gives a crapload of matches. A completely unscientific random
sampling suggests that the majority of them could make use of
`replace-region`, but that only a smaller proportion of them are likely
to benefit from the fancy diff of `replace-region-contents`.
See the (untested) patch below resulting from my random sampling (can
you spot which ones use the fancy diff algo and which ones don't).
To me the strongest argument in favor of having 2 separate functions is
that the semantics is sufficiently different that callers need to know
which is which (mostly because of the difference in algorithmic
complexity but also because of where point is (guaranteed or not) after
the call).
[ I noticed also that some of them could benefit from an `and-extract`
option (i.e. return the old text), e.g. in `transpose-subr-1`.
It would be easy/cheap to add such an option to `replace-region`, but not
to `replace-region-contents`. ]
>> Personally, I would feel reluctant to use this function. For example, I
>> don't know how to specify a meaningful value for MAX-COSTS. It is
>> documented as specifying "the quality of the difference computation",
>> which does not help much. I think our users will feel this even more.
>>
>> No other buffer editing primitives have anything resembling the MAX-SECS
>> and MAX-COSTS arguments. I find the design choice to expose those
>> arguments a bit unusual in a specialized function, and inappropriate in
>> a function intended for general use.
>
> The original version of replace-buffer-contents didn't have any such
> arguments. You called the function and had to wait for however long it
> took to do its job. We added these arguments to give Lisp programs
> more control, depending on the context and the importance of keeping
> the text which doesn't need to be changed. Now we are discussing how
> to give Lisp programs even more control.
[ Note: This sub-discussion is only very remotely related to
`replace-region`. ]
FWIW, I find the MAX-COSTS very problematic, because nobody knows what
it does. The only thing we know about it is that it's an integer and it
defaults to 10k. Based on the name we can assume that a larger value
pushes the limit further, so a smaller value may shorten the time to
complete (while reducing the quality of the output), but we have no idea
about the proportion. To make it usable, the programmer would need to
know things like:
- How does it relate to the size of the replaced text?
E.g. If the replacement is "zoomed" by a factor 4 (i.e. e.g. just
replace each char in both the old and the new text with 4 repetitions
of that char), does the same MAX-COSTS lead to finding the same diff
in both cases (modulo the zoom factor)?
- If I reduce MAX-COSTS by half, is the max runtime reduced by half?
Much less than half? Much more than half? Something else altogether?
>> That said, I understand that they are occasionally useful, or they
>> wouldn't exist. I would make them internal to the function, with
>> sensible defaults that users could override with some variable when they
>> need to. That change could be done in a backwards-compatible way with
>> advertised-calling-convention', and I would suggest we do that.
> I'm not sure I see how this would be better than what we have now.
I'd tend to agree: Dynbound implicit args aren't great, so I think the
current setup is not bad choice in this regard. The MAX-SECS arg tends
to depend on the specific call, AFAICT, so I'm not sure there'd be much
benefit to a global, overridable, default. As for MAX-COSTS, I don't
think we can guess what setup is best without being able to answer the
questions above.
Stefan
[replace-region.patch (text/x-diff, inline)]
diff --git a/admin/admin.el b/admin/admin.el
index 542556a65e0..a34e4adb9e1 100644
--- a/admin/admin.el
+++ b/admin/admin.el
@@ -517,8 +517,7 @@ manual-html-fix-headers
(setq opoint (point))
(search-forward "</head>")
(goto-char (match-beginning 0))
- (delete-region opoint (point))
- (insert manual-style-string)
+ (replace-region-contents opoint (point) manual-style-string 0)
;; Remove Texinfo 5 hard-coding bgcolor, text, link, vlink, alink.
(when (re-search-forward "<body lang=\"[^\"]+\"" nil t)
(setq opoint (point))
@@ -603,9 +602,10 @@ manual-html-fix-index-2
(setq opoint (match-beginning 0))
(while (and (looking-at " *—")
(zerop (forward-line 1))))
- (delete-region opoint (point))
- (insert "</table>\n\n\
-<h2>Detailed Node Listing</h2>\n\n<p>")
+ (replace-region-contents opoint (point)
+ "</table>\n\n\
+<h2>Detailed Node Listing</h2>\n\n<p>"
+ 0)
;; FIXME Fragile!
;; The Emacs and Elisp manual have some text at the
;; start of the detailed menu that is not part of the menu.
@@ -651,14 +651,15 @@ manual-html-fix-index-2
(save-excursion
(forward-char -1)
(skip-chars-backward " ")
- (delete-region (point) (line-end-position))
- (insert "</td>\n </tr>")))
- (insert " <tr>\n ")
- (if table-workaround
- ;; This works around a Firefox bug in the mono file.
- (insert "<td bgcolor=\"white\">")
- (insert "<td>"))
- (insert tag "</td>\n <td>" (or desc ""))
+ (replace-region-contents (point) (progn (end-of-line) (point))
+ "</td>\n </tr>" 0)
+ ))
+ (insert " <tr>\n "
+ (if table-workaround
+ ;; This works around a Firefox bug in the mono file.
+ "<td bgcolor=\"white\">"
+ "<td>")
+ tag "</td>\n <td>" (or desc ""))
(setq open-td t))
((eq (char-after) ?\n)
(delete-char 1)
diff --git a/admin/gitmerge.el b/admin/gitmerge.el
index 5bfb23dc3a2..ec2e5dd267b 100644
--- a/admin/gitmerge.el
+++ b/admin/gitmerge.el
@@ -324,12 +324,8 @@ gitmerge-resolve
;; match-3's first.
(let ((match3 (buffer-substring start3 end3))
(match1 (buffer-substring start1 end1)))
- (delete-region start3 end3)
- (goto-char start3)
- (insert match1)
- (delete-region start1 end1)
- (goto-char start1)
- (insert match3)))))
+ (replace-region-contents start3 end3 match1 0)
+ (replace-region-contents start1 end1 match3 0)))))
;; (pop-to-buffer (current-buffer)) (debug 'before-resolve)
))
;; Try to resolve the conflicts.
diff --git a/admin/tree-sitter/treesit-admin.el b/admin/tree-sitter/treesit-admin.el
index 86174ed2625..516fd7bb700 100644
--- a/admin/tree-sitter/treesit-admin.el
+++ b/admin/tree-sitter/treesit-admin.el
@@ -187,8 +187,7 @@ treesit-admin--verify-major-mode-queries
(forward-line -1))
(setq beg (point))
(search-forward "\n\n")
- (delete-region beg (point))
- (insert ";;\n")
+ (replace-region-contents beg (point) ";;\n" 0)
(dolist (mode modes)
(insert (format ";; %s has been tested with the following grammars and version:\n" mode))
(dolist (lang (alist-get mode mode-language-alist))
diff --git a/lisp/abbrev.el b/lisp/abbrev.el
index 93eb086da7a..cf3af3ea4e5 100644
--- a/lisp/abbrev.el
+++ b/lisp/abbrev.el
@@ -1106,9 +1106,9 @@ unexpand-abbrev
(unless (stringp val)
(error "Value of abbrev-symbol must be a string"))
;; Don't inherit properties here; just copy from old contents.
- (insert last-abbrev-text)
- ;; Delete after inserting, to better preserve markers.
- (delete-region (point) (+ (point) (length val)))
+ (forward-char (length val))
+ (replace-region-contents last-abbrev-location (point)
+ last-abbrev-text 0)
(setq last-abbrev-text nil))))))
(defun abbrev--write (sym)
diff --git a/lisp/mail/rmailedit.el b/lisp/mail/rmailedit.el
index 60916818caf..32b5e0d3da3 100644
--- a/lisp/mail/rmailedit.el
+++ b/lisp/mail/rmailedit.el
@@ -350,8 +350,7 @@ rmail-abort-edit
"Abort edit of current message; restore original contents."
(interactive)
(widen)
- (delete-region (point-min) (point-max))
- (insert rmail-old-text)
+ (replace-region-contents (point-min) (point-max) rmail-old-text)
(rmail-cease-edit t)
(rmail-highlight-headers))
diff --git a/lisp/progmodes/cperl-mode.el b/lisp/progmodes/cperl-mode.el
index 7052ac75817..d2379ecac78 100644
--- a/lisp/progmodes/cperl-mode.el
+++ b/lisp/progmodes/cperl-mode.el
@@ -5829,9 +5829,10 @@ cperl-fix-line-spacing
cperl-extra-newline-before-brace)))
(setq pp (point))
(skip-chars-forward " \t\n")
- (delete-region pp (point))
- (insert
- (make-string cperl-indent-region-fix-constructs ?\ )))
+ (replace-region-contents
+ pp (point)
+ (make-string cperl-indent-region-fix-constructs ?\ )
+ 0))
((and (looking-at "[\t ]*{")
(if ml cperl-extra-newline-before-brace-multiline
cperl-extra-newline-before-brace))
diff --git a/lisp/simple.el b/lisp/simple.el
index 7037158df8d..26513d52430 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -8834,18 +8834,19 @@ transpose-subr-1
(atomic-change-group
;; This sequence of insertions attempts to preserve marker
;; positions at the start and end of the transposed objects.
- (let* ((word (buffer-substring (car pos2) (cdr pos2)))
+ ;; FIXME: Why don't we use `transpose-regions', which has extra code
+ ;; to "transpose" the markers together with the text!?
+ (let* ((word2 (buffer-substring (car pos2) (cdr pos2)))
(len1 (- (cdr pos1) (car pos1)))
- (len2 (length word))
- (boundary (make-marker)))
- (set-marker boundary (car pos2))
+ (len2 (length word2))
+ (boundary (copy-marker (car pos2))))
(goto-char (cdr pos1))
- (insert-before-markers word)
- (setq word (delete-and-extract-region (car pos1) (+ (car pos1) len1)))
- (goto-char boundary)
- (insert word)
- (goto-char (+ boundary len1))
- (delete-region (point) (+ (point) len2))
+ ;; FIXME: We could use `replace-region-contents' but at the cost of
+ ;; not benefiting from `delete-and-extract-region'.
+ (insert-before-markers word2)
+ (let ((word1 (delete-and-extract-region (car pos1) (+ (car pos1) len1))))
+ (goto-char (+ boundary len2))
+ (replace-region-contents boundary (+ boundary len2) word1 0))
(set-marker boundary nil))))
(defun backward-word (&optional arg)
diff --git a/lisp/tar-mode.el b/lisp/tar-mode.el
index 3b9898bd2f4..a235dd4ab83 100644
--- a/lisp/tar-mode.el
+++ b/lisp/tar-mode.el
@@ -1425,31 +1425,30 @@ tar-alter-one-field
;;
;; update the header-line.
(let ((col (current-column)))
- (delete-region (line-beginning-position)
- (prog2 (forward-line 1)
- (point)
- ;; Insert the new text after the old, before deleting,
- ;; to preserve markers such as the window start.
- (insert (tar-header-block-summarize descriptor) "\n")))
- (forward-line -1) (move-to-column col))
+ (replace-region-contents
+ (line-beginning-position) (line-end-position)
+ (tar-header-block-summarize descriptor) 0)
+ (move-to-column col))
(cl-assert (tar-data-swapped-p))
(with-current-buffer tar-data-buffer
(let* ((start (- (tar-header-data-start descriptor) 512)))
- ;;
- ;; delete the old field and insert a new one.
- (goto-char (+ start data-position))
- (delete-region (point) (+ (point) (length new-data-string))) ; <--
- (cl-assert (not (or enable-multibyte-characters
- (multibyte-string-p new-data-string))))
- (insert new-data-string)
- ;;
- ;; compute a new checksum and insert it.
- (let ((chk (tar-header-block-checksum
+ ;;
+ (cl-assert (not (or enable-multibyte-characters
+ (multibyte-string-p new-data-string))))
+ ;; delete the old field and insert a new one.
+ (replace-region-contents
+ (+ start data-position)
+ (+ start data-position (length new-data-string))
+ new-data-string 0)
+ ;;
+ ;; compute a new checksum and insert it.
+ (let ((chk (tar-header-block-checksum
(buffer-substring start (+ start 512)))))
- (goto-char (+ start tar-chk-offset))
- (delete-region (point) (+ (point) 8))
- (insert (format "%6o\0 " chk))
+ (replace-region-contents
+ (+ start tar-chk-offset)
+ (+ start tar-chk-offset 8)
+ (format "%6o\0 " chk) 0)
(setf (tar-header-checksum descriptor) chk)
;;
;; ok, make sure we didn't botch it.
diff --git a/lisp/term.el b/lisp/term.el
index 862103d88e6..05001de098c 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -4054,7 +4054,6 @@ term-erase-in-line
(unless (and (eq saved-point (1- (point)))
(eq (char-before) ?\n)
(not wrapped))
- ;; Insert before deletion to preserve markers.
;; wrapped is true if we're at the beginning of screen line,
;; but not a buffer line. If we delete the current screen line
;; that will make the previous line no longer wrap, and (because
@@ -4064,10 +4063,8 @@ term-erase-in-line
;; contain a space, to force the previous line to continue to wrap.
;; We could do this always, but it seems preferable to not add the
;; extra space when wrapped is false.
- (when wrapped
- (insert-before-markers ? ))
- (insert-before-markers ?\n)
- (delete-region saved-point (point)))
+ (replace-region-contents saved-point (point)
+ (if wrapped " \n" "\n") 0))
(put-text-property saved-point (point) 'font-lock-face 'default)
(goto-char saved-point))))
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 459154f534b..84c735502be 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2139,9 +2139,8 @@ diff-apply-hunk
(t
;; Apply the hunk
(with-current-buffer buf
- (goto-char (car pos))
- (delete-region (car pos) (cdr pos))
- (insert (car new)))
+ (goto-char (cdr pos))
+ (replace-region-contents (car pos) (cdr pos) (car new)))
;; Display BUF in a window
(set-window-point (display-buffer buf) (+ (car pos) (cdr new)))
(diff-hunk-status-msg line-offset (xor switched reverse) nil)
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.