GNU bug report logs - #71525
30.0.50; Spin in delete-region/interval_deletion_adjustment

Previous Next

Full log

Message #44 received at 71525 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: jporterbugs <at> gmail.com, steven <at> stebalien.com, 71525 <at> debbugs.gnu.org
Subject: Re: bug#71525: 30.0.50; Spin in
 delete-region/interval_deletion_adjustment Spin in
 delete-region/interval_deletion_adjustment)
Date: Fri, 14 Jun 2024 10:13:30 +0300

> Date: Fri, 14 Jun 2024 02:41:15 +0300
> From: Dmitry Gutov <dmitry <at> gutov.dev>
> Cc: jporterbugs <at> gmail.com, 71525 <at> debbugs.gnu.org
> 
> On 14/06/2024 00:47, Dmitry Gutov wrote:
> > The thing is, decode_coding_c_string already calls 
> > adjust_markers_for_insert through
> > 
> >    decode_coding_object->decode_coding->produce_chars->insert_from_gap
> > 
> > And the extra call moves the markers too far.
> > 
> > Unfortunately, it's called with BEFORE_MARKERS=nil, and the above call 
> > chain makes it difficult to pass through the extra argument.
> 
> We can do it through the coding_system structure, though.
> 
> See attached. It fixes the freezes in my testing.
> 
> It seems like the least invasive possible fix, but better suggestions 
> welcome.

This is the correct approach, IMO.  But see some minor comments below.

> @@ -7814,7 +7815,7 @@ encode_coding (struct coding_system *coding)
>    } while (coding->consumed_char < coding->src_chars);
>  
>    if (BUFFERP (coding->dst_object) && coding->produced_char > 0)
> -    insert_from_gap (coding->produced_char, coding->produced, 0);
> +    insert_from_gap (coding->produced_char, coding->produced, 0, coding->dst_before_markers);

Here (and elsewhere in the patch) too-long lines should be broken in
two.

> +  /* True to insert before markers in the DST_OBJECT buffer.  */
> +  bool_bf dst_before_markers : 1;

I'd call this 'insert_before_markers' instead.  Please also add a
detailed comment here explaining what this flag is for and where and
why used.

>  /* Insert a sequence of NCHARS chars which occupy NBYTES bytes
>     starting at GAP_END_ADDR - NBYTES (if text_at_gap_tail) and at
> -   GPT_ADDR (if not text_at_gap_tail).  */
> +   GPT_ADDR (if not text_at_gap_tail).
> +
> +  If BEFORE_MARKERS is true, insert before markers. */

This commentary should also mention process.c as the single caller
using this facility in unconventional ways.

>  extern void insert_from_gap_1 (ptrdiff_t, ptrdiff_t, bool text_at_gap_tail);
> -extern void insert_from_gap (ptrdiff_t, ptrdiff_t, bool text_at_gap_tail);
> +extern void insert_from_gap (ptrdiff_t, ptrdiff_t, bool text_at_gap_tail,
> +			     bool before_markers);

We don't have names of arguments in prototypes, only their types.
(Yes, it means the original prototype, and the ones around it, were
also wrong.)

> --- a/src/process.c
> +++ b/src/process.c
> @@ -6415,6 +6415,7 @@ read_and_insert_process_output (struct Lisp_Process *p, char *buf,
>        specpdl_ref count1 = SPECPDL_INDEX ();
>  
>        XSETBUFFER (curbuf, current_buffer);
> +      process_coding->dst_before_markers = true;

Please also add a comment here explaining why this is done.

Thanks.

Previous Next

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