GNU bug report logs - #76313
New function `replace-region`

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: phst <at> google.com, 76313 <at> debbugs.gnu.org, stefankangas <at> gmail.com, tsdh <at> gnu.org
Subject: bug#76313: New function `replace-region`
Date: Sat, 29 Mar 2025 12:49:33 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Stefan Kangas <stefankangas <at> gmail.com>,  phst <at> google.com,
>   76313 <at> debbugs.gnu.org,  tsdh <at> gnu.org
> Date: Fri, 28 Mar 2025 00:56:45 -0400
> 
> I just pushed to `scratch/replace-region-contents` a small series of
> patches which consolidate `replace-buffer-contents` and
> `replace-region-contents`, marking `replace-buffer-contents` as obsolete
> and making `replace-region-contents` usable as a replacement for the
> `replace-region` I'd still prefer (the difference is just a mere
> `-contents` plus an extra `0` argument, but as argued here before, this
> sets a trap for the unwary user who may unwittingly end up calling
> a very expensive function).

Thanks, a few comments below.

> +@defun replace-region-contents beg end source &optional max-secs max-costs inherit
> +This function replaces the region between @var{beg} and @var{end}
> +of the current buffer with the text found in @var{source} which
> +is usually a string or a buffer, in which case it will use the
> +accessible portion of that buffer.
>  
>  This function attempts to keep point, markers, text properties, and
>  overlays in the current buffer intact.  One potential case where this
> -behavior is useful is external code formatting programs: they
> -typically write the reformatted text into a temporary buffer or file,
> -and using @code{delete-region} and @code{insert-buffer-substring}
> -would destroy these properties.  However, the latter combination is
> -typically faster (@xref{Deletion}, and @ref{Insertion}).
> -
> -For its working, @code{replace-buffer-contents} needs to compare the
> -contents of the original buffer with that of @var{source} which is a
> -costly operation if the buffers are huge and there is a high number of
> -differences between them.  In order to keep
> +behavior is useful is external code formatting programs: they typically
> +write the reformatted text into a temporary buffer or file, and using
> +@code{insert} and @code{delete-region} would destroy these properties.
> +
> +However, in order to do that, @code{replace-buffer-contents} needs to
                                       ^^^^^^^^^^^^^^^^^^^^^^^
The name of the function there is incorrect.

> +(defun replace-buffer-contents (source &optional max-secs max-costs)
> +  "Replace accessible portion of current buffer with that of SOURCE.
> +SOURCE can be a buffer or a string that names a buffer.
> +Interactively, prompt for SOURCE.
> +
> +The replacement is performed using `replace-region-contents'
> +which also describes the MAX-SECS and MAX-COSTS arguments and the
> +return value."
> +  (declare (obsolete replace-region-contents "31.1"))
> +  (interactive "bSource buffer: ")
> +  (replace-region-contents (point-min) (point-max)
> +                           (if (stringp source) (get-buffer source) source)
> +                           max-secs max-costs))

Since this is an interactive function, shouldn't it test SOURCE to be
of a valid type, instead of delegating to replace-region-contents?

> +Note: If the replacement is a string, it’ll usually be placed internally
> +in a temporary buffer.  Therefore, if you already have the replacement
> +in a buffer, it makes no sense to convert it to a string using
> +‘buffer-substring’ or similar.

This uses Unicode quotes in a doc string; please use ASCII characters
instead.

> +SOURCE can also be a function that will be called with no argument
                                                          ^^^^^^^^^^^
"no arguments", right?

> +and with current buffer narrowed to BEG..END and should return
                                               ^
A comma is missing there.

> +      b = XBUFFER (AREF (source, 0));
> +      min_b = XFIXNUM (AREF (source, 1));
> +      size_b = XFIXNUM (insend) - min_b;
> +      if (size_b < 0)
> +	error ("Negative sized source range");

Shouldn't we support here any order of SBEG and SEND, as we do in many
other places?

> +      if (! (BUF_BEGV (b) <= min_b && min_b + size_b <= BUF_ZV (b)))
> +	args_out_of_range_3 (AREF (source, 0), AREF (source, 0), AREF (source, 1));

Should this error message show all 3 values, instead of showing one of
them twice?  Or maybe show BEGV, ZV, and the offending position
(either SBEG or SEND)?

> +  bool b_empty = size_b == 0;
> +  if (b && !BUFFER_LIVE_P (b))
>      error ("Selecting deleted buffer");

What if b is NULL? can we continue?

> +  /* The rest of the code is not prepared to handle a string SOURCE.  */
> +  if (!b)
> +    {
> +      Lisp_Object name
> +	= Fgenerate_new_buffer_name (build_string (" *replace-workbuf*"), Qnil);
> +      Lisp_Object workbuf = Fget_buffer_create (name, Qt);
> +      b = XBUFFER (workbuf);
> +      record_unwind_protect (kill_buffer, workbuf);
> +      record_unwind_current_buffer ();
> +      set_buffer_internal (b);
> +      Fset_buffer_multibyte (STRING_MULTIBYTE (source) ? Qt : Qnil);

coding.c:code_conversion_save does something similar; perhaps some of
the buffer-local settings we use there could also be a good idea here?

> --- a/src/insdel.c
> +++ b/src/insdel.c
> @@ -1439,6 +1439,12 @@ replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new,
>        insbeg = 0;
>        inschars = SCHARS (new);
>      }
> +  else if (BUFFERP (new))
> +    {
> +      insbuf = XBUFFER (new);
> +      insbeg = BUF_BEGV (insbuf);
> +      inschars = BUF_ZV (insbuf) - insbeg;
> +    }

Please update the commentary to replace_range to describe this type of
NEW.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.