Inefficient code in reftex-index.el

Inefficient code in reftex-index.el

[Kim F. Storm]

I noticed the following code in reftex-index.el:

                    (condition-case nil (texmathp) (error nil))))
            (setq beg (car (match-data))
                  end (nth 1 (match-data)))

Using match-data like that seems inefficient.

I suggtest using match-beginning/match-end instead.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Inefficient code in reftex-index.el

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> I noticed the following code in reftex-index.el:
>
>                     (condition-case nil (texmathp) (error nil))))
>             (setq beg (car (match-data))
>                   end (nth 1 (match-data)))
>
> Using match-data like that seems inefficient.
>
> I suggtest using match-beginning/match-end instead.

That is not the same: the above will set beg and end to markers,
whereas match-beginning/match-end happen to be integers.  However, the
above will also create markers that are unused, so it would be saner
to call (match-data) only once _if_ indeed markers are what is
required.

Unused markers slow done editing operations afterwards.  So even if
markers _are_ what is wanted for some reason, they should be
explicitly unseated with set-marker once they are no longer needed.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Inefficient code in reftex-index.el

[Stefan Monnier]

> That is not the same: the above will set beg and end to markers,
> whereas match-beginning/match-end happen to be integers.  However, the

If you want markers, then say so: (make-marker (match-beginning N))


        Stefan

Re: Inefficient code in reftex-index.el

[Kim F. Storm]

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

>> That is not the same: the above will set beg and end to markers,
>> whereas match-beginning/match-end happen to be integers.  However, the
>
> If you want markers, then say so: (make-marker (match-beginning N))

Yes, indeed.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Inefficient code in reftex-index.el

[Juanma Barranquero]

On 6/6/05, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> If you want markers, then say so: (make-marker (match-beginning N))

  (copy-marker (match-beginning-N))

or

  (set-marker (make-marker) (match-beginning-N))

perhaps?

-- 
                    /L/e/k/t/u

Re: Inefficient code in reftex-index.el

[Kim F. Storm]

David Kastrup <dak@gnu.org> writes:

> That is not the same: the above will set beg and end to markers,

How "clever" ... I would NEVER have guessed that.

> whereas match-beginning/match-end happen to be integers.  However, the
> above will also create markers that are unused, so it would be saner
> to call (match-data) only once _if_ indeed markers are what is
> required.
>
> Unused markers slow done editing operations afterwards.  So even if
> markers _are_ what is wanted for some reason, they should be
> explicitly unseated with set-marker once they are no longer needed.


Which brings me to the suggestion that we add an optional arg to
set-match-data like this:

  (set-match-data list &optional destroy-markers)

and change save-match-data to use it 

(defmacro save-match-data (&rest body)
  "Execute the BODY forms, restoring the global value of the match data.
The value returned is the value of the last form in BODY."
  ;; It is better not to use backquote here,
  ;; because that makes a bootstrapping problem
  ;; if you need to recompile all the Lisp files using interpreted code.
  (declare (indent 0) (debug t))
  (list 'let
	'((save-match-data-internal (match-data)))
	(list 'unwind-protect
	      (cons 'progn body)
	      '(set-match-data save-match-data-internal t))))

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Inefficient code in reftex-index.el

[Carsten Dominik]

>>>>> "DK" == David Kastrup <dak@gnu.org> writes:

DK> storm@cua.dk (Kim F. Storm) writes:
>> I noticed the following code in reftex-index.el:
>> 
>> (condition-case nil (texmathp) (error nil))))
>> (setq beg (car (match-data))
>> end (nth 1 (match-data)))
>> 
>> Using match-data like that seems inefficient.
>> 
>> I suggtest using match-beginning/match-end instead.

DK> That is not the same: the above will set beg and end to markers,
DK> whereas match-beginning/match-end happen to be integers.  

Yes, indeed markers are needed here.  I will change it like Stafan
proposed it.  Thanks for picking this up, Kim.

I don't understand your ironic reply to Davids email.  Entirely
unnecessary.

- Carsten


--
Carsten Dominik <dominik@science.uva.nl>        \ _ /
Sterrenkundig Instituut "Anton Pannekoek"        |X|               _
Kruislaan 403; NL-1098 SJ Amsterdam             /| |\   _  _     _/ \
phone +31 (20) 525-7477; FAX +31 (20) 525-7484 __|o|___/ ~~ \___/    ~~~

Re: Inefficient code in reftex-index.el

[Kim F. Storm]

storm@cua.dk (Kim F. Storm) writes:

>
> Which brings me to the suggestion that we add an optional arg to
> set-match-data like this:
>
>   (set-match-data list &optional destroy-markers)
>
> and change save-match-data to use it 
>
> (defmacro save-match-data (&rest body)
>   "Execute the BODY forms, restoring the global value of the match data.
> The value returned is the value of the last form in BODY."
>   ;; It is better not to use backquote here,
>   ;; because that makes a bootstrapping problem
>   ;; if you need to recompile all the Lisp files using interpreted code.
>   (declare (indent 0) (debug t))
>   (list 'let
> 	'((save-match-data-internal (match-data)))
> 	(list 'unwind-protect
> 	      (cons 'progn body)
> 	      '(set-match-data save-match-data-internal t))))

I made the change slightly different to avoid adding another arg to
Fset_match_data, which causes problems at C level.  Instead, if first
arg to match-data is 'evaporate', set-match-data will eventually free
markers on the resulting list...

Here is a patch to implement this, and use it at the C-level, 
and for save-match-data:

I don't know for sure, but emacs feels a tiny bit more responsive
with this change!


Index: src/composite.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/composite.c,v
retrieving revision 1.31
diff -c -r1.31 composite.c
*** src/composite.c	26 Dec 2003 11:39:22 -0000	1.31
--- src/composite.c	6 Jun 2005 22:19:14 -0000
***************
*** 628,634 ****
      }
  
    /* Preserve the match data.  */
!   record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
  
    /* If none of ASCII characters have composition functions, we can
       skip them quickly.  */
--- 628,634 ----
      }
  
    /* Preserve the match data.  */
!   record_unwind_protect (Fset_match_data, Fmatch_data (Qevaporate, Qnil));
  
    /* If none of ASCII characters have composition functions, we can
       skip them quickly.  */
Index: src/eval.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/eval.c,v
retrieving revision 1.240
diff -c -r1.240 eval.c
*** src/eval.c	3 Jun 2005 23:02:21 -0000	1.240
--- src/eval.c	6 Jun 2005 22:19:15 -0000
***************
*** 1971,1977 ****
    GCPRO3 (fun, funname, fundef);
  
    /* Preserve the match data.  */
!   record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
  
    /* Value saved here is to be restored into Vautoload_queue.  */
    record_unwind_protect (un_autoload, Vautoload_queue);
--- 1971,1977 ----
    GCPRO3 (fun, funname, fundef);
  
    /* Preserve the match data.  */
!   record_unwind_protect (Fset_match_data, Fmatch_data (Qevaporate, Qnil));
  
    /* Value saved here is to be restored into Vautoload_queue.  */
    record_unwind_protect (un_autoload, Vautoload_queue);
cvs diff: I know nothing about src/lisp.c
Index: src/macmenu.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/macmenu.c,v
retrieving revision 1.28
diff -c -r1.28 macmenu.c
*** src/macmenu.c	6 Jun 2005 20:24:13 -0000	1.28
--- src/macmenu.c	6 Jun 2005 22:19:15 -0000
***************
*** 1475,1481 ****
  	 because it is not reentrant.  */
        specbind (Qdebug_on_next_call, Qnil);
  
!       record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
        if (NILP (Voverriding_local_map_menu_flag))
  	{
  	  specbind (Qoverriding_terminal_local_map, Qnil);
--- 1475,1481 ----
  	 because it is not reentrant.  */
        specbind (Qdebug_on_next_call, Qnil);
  
!       record_unwind_protect (Fset_match_data, Fmatch_data (Qevaporate, Qnil));
        if (NILP (Voverriding_local_map_menu_flag))
  	{
  	  specbind (Qoverriding_terminal_local_map, Qnil);
Index: src/search.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/search.c,v
retrieving revision 1.192
diff -c -r1.192 search.c
*** src/search.c	20 Apr 2005 07:21:47 -0000	1.192
--- src/search.c	6 Jun 2005 22:19:15 -0000
***************
*** 2751,2777 ****
  this case, and if the last match was in a buffer, the buffer will get
  stored as one additional element at the end of the list.
  
! If REUSE is a list, reuse it as part of the value.  If REUSE is long enough
! to hold all the values, and if INTEGERS is non-nil, no consing is done.
  
  Return value is undefined if the last search failed.  */)
!      (integers, reuse)
       Lisp_Object integers, reuse;
  {
    Lisp_Object tail, prev;
    Lisp_Object *data;
!   int i, len;
  
    if (NILP (last_thing_searched))
      return Qnil;
  
    prev = Qnil;
  
!   data = (Lisp_Object *) alloca ((2 * search_regs.num_regs + 1)
  				 * sizeof (Lisp_Object));
  
    len = 0;
!   for (i = 0; i < search_regs.num_regs; i++)
      {
        int start = search_regs.start[i];
        if (start >= 0)
--- 2751,2803 ----
  this case, and if the last match was in a buffer, the buffer will get
  stored as one additional element at the end of the list.
  
! If INTEGER is the symbol `evaporate', mark the returned list so that
! any markers on the list shall be automatically freed when passed to
! `set-match-data'.
! 
! If REUSE is a list, reuse it as part of the value.  If REUSE is long
! enough to hold all the values, and if INTEGERS is non-nil, no consing
! is done.
! Note: Any previous markers on the reuse list will be modified to point
! to nowhere.
  
  Return value is undefined if the last search failed.  */)
!     (integers, reuse)
       Lisp_Object integers, reuse;
  {
    Lisp_Object tail, prev;
    Lisp_Object *data;
!   int i, j, len;
  
    if (NILP (last_thing_searched))
      return Qnil;
  
    prev = Qnil;
  
!   for (tail = reuse; CONSP (tail); tail = XCDR (tail))
!     if (MARKERP (XCAR (tail)))
! 	{
! 	  unchain_marker (XMARKER (XCAR (tail)));
! 	  XSETCAR (tail, Qnil);
! 	}
! 
!   data = (Lisp_Object *) alloca ((2 * search_regs.num_regs + 2)
  				 * sizeof (Lisp_Object));
  
+   j = 0;
    len = 0;
! 
!   if (EQ (integers, Qevaporate))
!     {
!       if (BUFFERP (last_thing_searched))
! 	{
! 	  data[j++] = Qevaporate;
! 	  len++;
! 	}
!       integers = Qnil;
!     }
! 
!   for (i = 0; i < search_regs.num_regs; i++, j += 2)
      {
        int start = search_regs.start[i];
        if (start >= 0)
***************
*** 2779,2795 ****
  	  if (EQ (last_thing_searched, Qt)
  	      || ! NILP (integers))
  	    {
! 	      XSETFASTINT (data[2 * i], start);
! 	      XSETFASTINT (data[2 * i + 1], search_regs.end[i]);
  	    }
  	  else if (BUFFERP (last_thing_searched))
  	    {
! 	      data[2 * i] = Fmake_marker ();
! 	      Fset_marker (data[2 * i],
  			   make_number (start),
  			   last_thing_searched);
! 	      data[2 * i + 1] = Fmake_marker ();
! 	      Fset_marker (data[2 * i + 1],
  			   make_number (search_regs.end[i]),
  			   last_thing_searched);
  	    }
--- 2805,2821 ----
  	  if (EQ (last_thing_searched, Qt)
  	      || ! NILP (integers))
  	    {
! 	      XSETFASTINT (data[j], start);
! 	      XSETFASTINT (data[j + 1], search_regs.end[i]);
  	    }
  	  else if (BUFFERP (last_thing_searched))
  	    {
! 	      data[j] = Fmake_marker ();
! 	      Fset_marker (data[j],
  			   make_number (start),
  			   last_thing_searched);
! 	      data[j + 1] = Fmake_marker ();
! 	      Fset_marker (data[j + 1],
  			   make_number (search_regs.end[i]),
  			   last_thing_searched);
  	    }
***************
*** 2797,2806 ****
  	    /* last_thing_searched must always be Qt, a buffer, or Qnil.  */
  	    abort ();
  
! 	  len = 2*(i+1);
  	}
        else
! 	data[2 * i] = data [2 * i + 1] = Qnil;
      }
  
    if (BUFFERP (last_thing_searched) && !NILP (integers))
--- 2823,2832 ----
  	    /* last_thing_searched must always be Qt, a buffer, or Qnil.  */
  	    abort ();
  
! 	  len = j + 2;
  	}
        else
! 	data[j] = data[j + 1] = Qnil;
      }
  
    if (BUFFERP (last_thing_searched) && !NILP (integers))
***************
*** 2836,2847 ****
  
  DEFUN ("set-match-data", Fset_match_data, Sset_match_data, 1, 1, 0,
         doc: /* Set internal data on last search match from elements of LIST.
! LIST should have been created by calling `match-data' previously.  */)
!      (list)
       register Lisp_Object list;
  {
    register int i;
    register Lisp_Object marker;
  
    if (running_asynch_code)
      save_search_regs ();
--- 2862,2878 ----
  
  DEFUN ("set-match-data", Fset_match_data, Sset_match_data, 1, 1, 0,
         doc: /* Set internal data on last search match from elements of LIST.
! LIST should have been created by calling `match-data' previously.
! 
! If CLEAR-MARKERS is non-nil, make markers on the list point nowhere.
! If value is `evaporate', put the markers back on the free list; the
! caller must ensure that there are no other references to these markers.  */)
!     (list)
       register Lisp_Object list;
  {
    register int i;
    register Lisp_Object marker;
+   int free_markers = 0;
  
    if (running_asynch_code)
      save_search_regs ();
***************
*** 2853,2858 ****
--- 2884,2897 ----
       in LIST, assume that this match data came from a string.  */
    last_thing_searched = Qt;
  
+   /* If first element of list is `evaporate', free all markers
+      on the list after restoring search registers.  */
+   if (CONSP (list) && EQ (XCAR (list), Qevaporate))
+     {
+       free_markers = 1;
+       list = XCDR (list);
+     }
+ 
    /* Allocate registers if they don't already exist.  */
    {
      int length = XFASTINT (Flength (list)) / 2;
***************
*** 2882,2890 ****
  	search_regs.num_regs = length;
        }
  
!     for (i = 0;; i++)
        {
! 	marker = Fcar (list);
  	if (BUFFERP (marker))
  	  {
  	    last_thing_searched = marker;
--- 2921,2929 ----
  	search_regs.num_regs = length;
        }
  
!     for (i = 0; CONSP (list); i++)
        {
! 	marker = XCAR (list);
  	if (BUFFERP (marker))
  	  {
  	    last_thing_searched = marker;
***************
*** 2900,2906 ****
--- 2939,2947 ----
  	else
  	  {
  	    int from;
+ 	    Lisp_Object m;
  
+ 	    m = marker;
  	    if (MARKERP (marker))
  	      {
  		if (XMARKER (marker)->buffer == 0)
***************
*** 2911,2927 ****
  
  	    CHECK_NUMBER_COERCE_MARKER (marker);
  	    from = XINT (marker);
- 	    list = Fcdr (list);
  
! 	    marker = Fcar (list);
  	    if (MARKERP (marker) && XMARKER (marker)->buffer == 0)
  	      XSETFASTINT (marker, 0);
  
  	    CHECK_NUMBER_COERCE_MARKER (marker);
  	    search_regs.start[i] = from;
  	    search_regs.end[i] = XINT (marker);
  	  }
! 	list = Fcdr (list);
        }
  
      for (; i < search_regs.num_regs; i++)
--- 2952,2985 ----
  
  	    CHECK_NUMBER_COERCE_MARKER (marker);
  	    from = XINT (marker);
  
! 	    if (free_markers && MARKERP (m))
! 	      {
! 		XSETCAR (list, Qnil);
! 		free_marker (m);
! 	      }
! 
! 	    list = XCDR (list);
! 
! 	    if (!CONSP (list))
! 	      break;
! 
! 	    m = marker = Fcar (list);
! 
  	    if (MARKERP (marker) && XMARKER (marker)->buffer == 0)
  	      XSETFASTINT (marker, 0);
  
  	    CHECK_NUMBER_COERCE_MARKER (marker);
  	    search_regs.start[i] = from;
  	    search_regs.end[i] = XINT (marker);
+ 
+ 	    if (free_markers && MARKERP (m))
+ 	      {
+ 		XSETCAR (list, Qnil);
+ 		free_marker (m);
+ 	      }
  	  }
! 	list = XCDR (list);
        }
  
      for (; i < search_regs.num_regs; i++)
Index: src/w32menu.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/w32menu.c,v
retrieving revision 1.72
diff -c -r1.72 w32menu.c
*** src/w32menu.c	24 May 2005 08:44:25 -0000	1.72
--- src/w32menu.c	6 Jun 2005 22:19:15 -0000
***************
*** 1443,1449 ****
  	 because it is not reentrant.  */
        specbind (Qdebug_on_next_call, Qnil);
  
!       record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
        if (NILP (Voverriding_local_map_menu_flag))
  	{
  	  specbind (Qoverriding_terminal_local_map, Qnil);
--- 1443,1449 ----
  	 because it is not reentrant.  */
        specbind (Qdebug_on_next_call, Qnil);
  
!       record_unwind_protect (Fset_match_data, Fmatch_data (Qevaporate, Qnil));
        if (NILP (Voverriding_local_map_menu_flag))
  	{
  	  specbind (Qoverriding_terminal_local_map, Qnil);
Index: src/xdisp.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/xdisp.c,v
retrieving revision 1.1019
diff -c -r1.1019 xdisp.c
*** src/xdisp.c	6 Jun 2005 12:36:29 -0000	1.1019
--- src/xdisp.c	6 Jun 2005 22:19:17 -0000
***************
*** 8458,8464 ****
        Lisp_Object tail, frame;
        int count = SPECPDL_INDEX ();
  
!       record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
  
        FOR_EACH_FRAME (tail, frame)
  	{
--- 8458,8464 ----
        Lisp_Object tail, frame;
        int count = SPECPDL_INDEX ();
  
!       record_unwind_protect (Fset_match_data, Fmatch_data (Qevaporate, Qnil));
  
        FOR_EACH_FRAME (tail, frame)
  	{
***************
*** 8581,8587 ****
  
  	  set_buffer_internal_1 (XBUFFER (w->buffer));
  	  if (save_match_data)
! 	    record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
  	  if (NILP (Voverriding_local_map_menu_flag))
  	    {
  	      specbind (Qoverriding_terminal_local_map, Qnil);
--- 8581,8587 ----
  
  	  set_buffer_internal_1 (XBUFFER (w->buffer));
  	  if (save_match_data)
! 	    record_unwind_protect (Fset_match_data, Fmatch_data (Qevaporate, Qnil));
  	  if (NILP (Voverriding_local_map_menu_flag))
  	    {
  	      specbind (Qoverriding_terminal_local_map, Qnil);
***************
*** 8772,8778 ****
  
  	  /* Save match data, if we must.  */
  	  if (save_match_data)
! 	    record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
  
  	  /* Make sure that we don't accidentally use bogus keymaps.  */
  	  if (NILP (Voverriding_local_map_menu_flag))
--- 8772,8778 ----
  
  	  /* Save match data, if we must.  */
  	  if (save_match_data)
! 	    record_unwind_protect (Fset_match_data, Fmatch_data (Qevaporate, Qnil));
  
  	  /* Make sure that we don't accidentally use bogus keymaps.  */
  	  if (NILP (Voverriding_local_map_menu_flag))
Index: src/xmenu.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/xmenu.c,v
retrieving revision 1.292
diff -c -r1.292 xmenu.c
*** src/xmenu.c	6 Jun 2005 12:56:53 -0000	1.292
--- src/xmenu.c	6 Jun 2005 22:19:18 -0000
***************
*** 2030,2036 ****
  	 because it is not reentrant.  */
        specbind (Qdebug_on_next_call, Qnil);
  
!       record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
        record_unwind_protect (unuse_menu_items, Qnil);
        if (NILP (Voverriding_local_map_menu_flag))
  	{
--- 2030,2036 ----
  	 because it is not reentrant.  */
        specbind (Qdebug_on_next_call, Qnil);
  
!       record_unwind_protect (Fset_match_data, Fmatch_data (Qevaporate, Qnil));
        record_unwind_protect (unuse_menu_items, Qnil);
        if (NILP (Voverriding_local_map_menu_flag))
  	{
Index: lisp/subr.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/subr.el,v
retrieving revision 1.459
diff -c -r1.459 subr.el
*** lisp/subr.el	29 May 2005 08:34:46 -0000	1.459
--- lisp/subr.el	6 Jun 2005 22:19:18 -0000
***************
*** 1967,1973 ****
    ;; if you need to recompile all the Lisp files using interpreted code.
    (declare (indent 0) (debug t))
    (list 'let
! 	'((save-match-data-internal (match-data)))
  	(list 'unwind-protect
  	      (cons 'progn body)
  	      '(set-match-data save-match-data-internal))))
--- 1967,1973 ----
    ;; if you need to recompile all the Lisp files using interpreted code.
    (declare (indent 0) (debug t))
    (list 'let
! 	'((save-match-data-internal (match-data 'evaporate)))
  	(list 'unwind-protect
  	      (cons 'progn body)
  	      '(set-match-data save-match-data-internal))))

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Inefficient code in reftex-index.el

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> storm@cua.dk (Kim F. Storm) writes:
>
>>
>> Which brings me to the suggestion that we add an optional arg to
>> set-match-data like this:
>>
>>   (set-match-data list &optional destroy-markers)
>>
>> and change save-match-data to use it 
>>
>> (defmacro save-match-data (&rest body)
>>   "Execute the BODY forms, restoring the global value of the match data.
>> The value returned is the value of the last form in BODY."
>>   ;; It is better not to use backquote here,
>>   ;; because that makes a bootstrapping problem
>>   ;; if you need to recompile all the Lisp files using interpreted code.
>>   (declare (indent 0) (debug t))
>>   (list 'let
>> 	'((save-match-data-internal (match-data)))
>> 	(list 'unwind-protect
>> 	      (cons 'progn body)
>> 	      '(set-match-data save-match-data-internal t))))
>
> I made the change slightly different to avoid adding another arg to
> Fset_match_data, which causes problems at C level.  Instead, if first
> arg to match-data is 'evaporate', set-match-data will eventually free
> markers on the resulting list...

So if INTEGERS is 'evaporate, we don't get integers but markers, and
those will be unseated at restore time.

In addition, the list will get prolonged by one additional member.
This is probably not too tragic since I don't see anybody else
accessing a data structure created with (set-match-data 'evaporate),
but it is hardly an element of beauty.  Should not
(match-data whatever '(evaporate marker1 marker2 ...)) also clean
up the markers passed in for reuse?

Anyway, allow me to throw a fit.  The whole match-data thing was ugly
previously, but this surely takes the cake, platter and all.

At the very least, the INTEGERS argument should be renamed to
something more in line with the new function.  But I don't like that
interface at all, really.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Inefficient code in reftex-index.el

[Kim F. Storm]

David Kastrup <dak@gnu.org> writes:

>> I made the change slightly different to avoid adding another arg to
>> Fset_match_data, which causes problems at C level.  Instead, if first
>> arg to match-data is 'evaporate', set-match-data will eventually free
>> markers on the resulting list...
>
> So if INTEGERS is 'evaporate, we don't get integers but markers, and
> those will be unseated at restore time.

Right -- either you want integers or you want markers (that evaporates).
There is no ambiguity in that respect!

>
> In addition, the list will get prolonged by one additional member.
> This is probably not too tragic since I don't see anybody else
> accessing a data structure created with (set-match-data 'evaporate),

That's (match-data 'evaporate)...

> but it is hardly an element of beauty.  

The new interface is fully backwards compatible -- so unless the user
specifies 'evaporate, the list is formatted exactly as it was before.

>                                         Should not
> (match-data whatever '(evaporate marker1 marker2 ...)) also clean
> up the markers passed in for reuse?

It should, yes.  Currently, it just reseats the markers on the list.

> Anyway, allow me to throw a fit.  The whole match-data thing was ugly
> previously, but this surely takes the cake, platter and all.

What are the alternatives?

I can add new optional arguments to both match-data and set-match-data
to indicate what to do with markers in the REUSE and LIST arguments.
I can do that if people think it is better/cleaner. 

The drawback is that it requires more changes at the C-level -- but
it may actually clean-up a few things, so perhaps it would be good...

In any case, to me, the match-data interface should not be considered
a user-level feature _at all_.  The user level feature is
save-match-data, which does not (in principle) allow the user to mess
with the saved data.  And in that respect, nobody should really care
about what the format of the match data is.

IMO, the byte-compiler should warn about using match-data.

>
> At the very least, the INTEGERS argument should be renamed to
> something more in line with the new function.  

I will do that, if the current interface is acceptable.

>                                                But I don't like that
> interface at all, really.

I'm not excited about it either...  but it does the job.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Inefficient code in reftex-index.el

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> David Kastrup <dak@gnu.org> writes:
>
>>> I made the change slightly different to avoid adding another arg to
>>> Fset_match_data, which causes problems at C level.  Instead, if first
>>> arg to match-data is 'evaporate', set-match-data will eventually free
>>> markers on the resulting list...
>>
>> So if INTEGERS is 'evaporate, we don't get integers but markers, and
>> those will be unseated at restore time.
>
> Right -- either you want integers or you want markers (that evaporates).
> There is no ambiguity in that respect!
>
>>
>> In addition, the list will get prolonged by one additional member.
>> This is probably not too tragic since I don't see anybody else
>> accessing a data structure created with (set-match-data 'evaporate),
>
> That's (match-data 'evaporate)...
>
>> but it is hardly an element of beauty.  
>
> The new interface is fully backwards compatible -- so unless the user
> specifies 'evaporate, the list is formatted exactly as it was
> before.

Anyway, match-data states somehing like

    match-data is a built-in function in `src/search.c'.
    (match-data &optional INTEGERS REUSE)

    Return a list containing all info on what the last search matched.
    Element 2N is `(match-beginning N)'; element 2N + 1 is `(match-end N)'.
    All the elements are markers or nil (nil if the Nth pair didn't match)
    if the last match was on a buffer; integers or nil if a string was matched.
    Use `store-match-data' to reinstate the data in this list.

    If INTEGERS (the optional first argument) is non-nil, always use
    integers (rather than markers) to represent buffer positions.  In
    this case, and if the last match was in a buffer, the buffer will get
    stored as one additional element at the end of the list.

Notice the last sentence.  This keeps the description for Element 2N
and Element 2N+1 valid.

I think that, if at all, the evaporate thingy should also occupy the
last element on the list rather than the first.  We got enough
inconsistency already as it is.

>> Should not (match-data whatever '(evaporate marker1 marker2 ...))
>> also clean up the markers passed in for reuse?
>
> It should, yes.  Currently, it just reseats the markers on the list.

Oh, it does?  I thought that it just recycled the CONS cells, but not
the markers.

> In any case, to me, the match-data interface should not be
> considered a user-level feature _at all_.

There is no other interface into the number of accessible match
strings (which might be nil) rather than
(/ (length (match-data t)) 2).

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Inefficient code in reftex-index.el

[Kim F. Storm]

David Kastrup <dak@gnu.org> writes:

>     If INTEGERS (the optional first argument) is non-nil, always use
>     integers (rather than markers) to represent buffer positions.  In
>     this case, and if the last match was in a buffer, the buffer will get
>     stored as one additional element at the end of the list.
>
> Notice the last sentence.  This keeps the description for Element 2N
> and Element 2N+1 valid.
>
> I think that, if at all, the evaporate thingy should also occupy the
> last element on the list rather than the first.  We got enough
> inconsistency already as it is.

That's probably a good idea -- I'll give it a try.

>
>>> Should not (match-data whatever '(evaporate marker1 marker2 ...))
>>> also clean up the markers passed in for reuse?
>>
>> It should, yes.  Currently, it just reseats the markers on the list.
>
> Oh, it does?  I thought that it just recycled the CONS cells, but not
> the markers.

It used to do that, but my patch changed it to reseat the markers.

In any case, I didn't find any code which actually uses the REUSE
form of match-data, so that feature is pretty obscure already, so
I think we should just keep the original behavior of the REUSE arg.

>
>> In any case, to me, the match-data interface should not be
>> considered a user-level feature _at all_.
>
> There is no other interface into the number of accessible match
> strings (which might be nil) rather than
> (/ (length (match-data t)) 2).

That's still pretty inefficient -- I suggest that we introduce a new
function `match-count' to return that number.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Inefficient code in reftex-index.el

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> In any case, I didn't find any code which actually uses the REUSE
> form of match-data, so that feature is pretty obscure already, so I
> think we should just keep the original behavior of the REUSE arg.

Look at replace-match-data in replace.el...

I think it was me that did that piece of prescient paranoia.

>>> In any case, to me, the match-data interface should not be
>>> considered a user-level feature _at all_.
>>
>> There is no other interface into the number of accessible match
>> strings (which might be nil) rather than
>> (/ (length (match-data t)) 2).
>
> That's still pretty inefficient -- I suggest that we introduce a new
> function `match-count' to return that number.

No objection here.  No idea whether match-size would be more
appropriate.  The C code is not really helpful deciding about that
since it uses search_regs.num_regs and that is not suggestive of any
sensible Lisp name.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Inefficient code in reftex-index.el

[Kim F. Storm]

David Kastrup <dak@gnu.org> writes:

> storm@cua.dk (Kim F. Storm) writes:
>
>> In any case, I didn't find any code which actually uses the REUSE
>> form of match-data, so that feature is pretty obscure already, so I
>> think we should just keep the original behavior of the REUSE arg.
>
> Look at replace-match-data in replace.el...

I see.  But that doesn't contradict my feeling that match-data
doesn't have to do anything special about markers in REUSE.

But we could make an optional arg RESEAT to match-data which
causes the markers in REUSE to be reseated to nowhere (if t),
or be freed if RESEAT equals `evaporate'.

>
> I think it was me that did that piece of prescient paranoia.
>
>>>> In any case, to me, the match-data interface should not be
>>>> considered a user-level feature _at all_.
>>>
>>> There is no other interface into the number of accessible match
>>> strings (which might be nil) rather than
>>> (/ (length (match-data t)) 2).
>>
>> That's still pretty inefficient -- I suggest that we introduce a new
>> function `match-count' to return that number.
>
> No objection here.  No idea whether match-size would be more
> appropriate.  The C code is not really helpful deciding about that
> since it uses search_regs.num_regs and that is not suggestive of any
> sensible Lisp name.

IMO, match-count is better than match-size.

BTW, match-count != search_regs.num_regs, as match-data only
returns elements up to the last successful submatch.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Inefficient code in reftex-index.el

[Richard Stallman]

    Which brings me to the suggestion that we add an optional arg to
    set-match-data like this:

      (set-match-data list &optional destroy-markers)

    and change save-match-data to use it 

That would be an improvement, but the next GC will destroy these
markers anyway, so it may not be a big improvement except in code that
does this an awful lot.

Re: Inefficient code in reftex-index.el

[Richard Stallman]

    In any case, to me, the match-data interface should not be considered
    a user-level feature _at all_.

I don't agree.  It is legitimate for user code to call match-data
directly.  There is no reason for the changes people are proposing.

Re: Inefficient code in reftex-index.el

[Richard Stallman]

    > There is no other interface into the number of accessible match
    > strings (which might be nil) rather than
    > (/ (length (match-data t)) 2).

    That's still pretty inefficient -- I suggest that we introduce a new
    function `match-count' to return that number.

Is there sufficient use for this function to justify introducing it?
I think that most cases where this would be used, the code would
then proceed to call match-data.

Re: Inefficient code in reftex-index.el

[David Kastrup]

Richard Stallman <rms@gnu.org> writes:

>     In any case, to me, the match-data interface should not be considered
>     a user-level feature _at all_.
>
> I don't agree.  It is legitimate for user code to call match-data
> directly.  There is no reason for the changes people are proposing.

Problem is that markers slow down editing, and significantly so.  And
normal editing operations are not associated with extensive consing,
so they won't trigger frequent garbage collection.

It is not good that save-match-data leaves markers lying around, and
there is little evidence that user code using match-data is written
with the issues in mind.

Providing ways to deal with the issues right with the functions
themselves might make it both more obvious and convenient to people to
recognize and do the right thing.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Inefficient code in reftex-index.el

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     In any case, to me, the match-data interface should not be considered
>     a user-level feature _at all_.
>
> I don't agree.  It is legitimate for user code to call match-data
> directly.  

In special cases, yes.  

However, the majority of code should just use save-match-data.

>            There is no reason for the changes people are proposing.

IMO, getting rid of unused markers asap is a good thing.

Here is a different approach, adding an optional RESEAT arg to
match-data and set-match-data as I originally envisioned:

The majority of the patch is due to intrucing a new
record_unwind_save_match_data function and renaming restore_match_data
to restore_search_regs (for analogy with save_search_regs).

I didn't implement "match-count" in this patch -- it would be trivial
to add.

Index: subr.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/subr.el,v
retrieving revision 1.459
diff -c -r1.459 subr.el
*** subr.el	29 May 2005 08:34:46 -0000	1.459
--- subr.el	7 Jun 2005 14:40:17 -0000
***************
*** 1970,1976 ****
  	'((save-match-data-internal (match-data)))
  	(list 'unwind-protect
  	      (cons 'progn body)
! 	      '(set-match-data save-match-data-internal))))
  
  (defun match-string (num &optional string)
    "Return string of text matched by last search.
--- 1970,1976 ----
  	'((save-match-data-internal (match-data)))
  	(list 'unwind-protect
  	      (cons 'progn body)
! 	      '(set-match-data save-match-data-internal t))))
  
  (defun match-string (num &optional string)
    "Return string of text matched by last search.
Index: replace.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/replace.el,v
retrieving revision 1.211
diff -c -r1.211 replace.el
*** replace.el	26 May 2005 13:08:57 -0000	1.211
--- replace.el	7 Jun 2005 14:40:17 -0000
***************
*** 1268,1279 ****
  	     (and (eq new reuse)
  		  (eq (null integers) (markerp (car reuse)))
  		  new)))
!       (match-data integers
! 		  (prog1 reuse
! 		    (while reuse
! 		      (if (markerp (car reuse))
! 			  (set-marker (car reuse) nil))
! 		      (setq reuse (cdr reuse)))))))
  
  (defun replace-match-maybe-edit (newtext fixedcase literal noedit match-data)
    "Make a replacement with `replace-match', editing `\\?'.
--- 1268,1274 ----
  	     (and (eq new reuse)
  		  (eq (null integers) (markerp (car reuse)))
  		  new)))
!       (match-data integers reuse t)))
  
  (defun replace-match-maybe-edit (newtext fixedcase literal noedit match-data)
    "Make a replacement with `replace-match', editing `\\?'.


Index: search.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/search.c,v
retrieving revision 1.192
diff -c -r1.192 search.c
*** search.c	20 Apr 2005 07:21:47 -0000	1.192
--- search.c	7 Jun 2005 14:28:53 -0000
***************
*** 2739,2745 ****
    return match_limit (subexp, 0);
  }
  
! DEFUN ("match-data", Fmatch_data, Smatch_data, 0, 2, 0,
         doc: /* Return a list containing all info on what the last search matched.
  Element 2N is `(match-beginning N)'; element 2N + 1 is `(match-end N)'.
  All the elements are markers or nil (nil if the Nth pair didn't match)
--- 2739,2745 ----
    return match_limit (subexp, 0);
  }
  
! DEFUN ("match-data", Fmatch_data, Smatch_data, 0, 3, 0,
         doc: /* Return a list containing all info on what the last search matched.
  Element 2N is `(match-beginning N)'; element 2N + 1 is `(match-end N)'.
  All the elements are markers or nil (nil if the Nth pair didn't match)
***************
*** 2751,2767 ****
  this case, and if the last match was in a buffer, the buffer will get
  stored as one additional element at the end of the list.
  
! If REUSE is a list, reuse it as part of the value.  If REUSE is long enough
! to hold all the values, and if INTEGERS is non-nil, no consing is done.
  
  Return value is undefined if the last search failed.  */)
!      (integers, reuse)
!      Lisp_Object integers, reuse;
  {
    Lisp_Object tail, prev;
    Lisp_Object *data;
    int i, len;
  
    if (NILP (last_thing_searched))
      return Qnil;
  
--- 2751,2779 ----
  this case, and if the last match was in a buffer, the buffer will get
  stored as one additional element at the end of the list.
  
! If REUSE is a list, reuse it as part of the value.  If REUSE is long
! enough to hold all the values, and if INTEGERS is non-nil, no consing
! is done.
! 
! If optional third arg RESEAT is non-nil, any previous markers on the
! REUSE list will be modified to point to nowhere.
  
  Return value is undefined if the last search failed.  */)
!   (integers, reuse, reseat)
!      Lisp_Object integers, reuse, reseat;
  {
    Lisp_Object tail, prev;
    Lisp_Object *data;
    int i, len;
  
+   if (!NILP (reseat))
+     for (tail = reuse; CONSP (tail); tail = XCDR (tail))
+       if (MARKERP (XCAR (tail)))
+ 	{
+ 	  unchain_marker (XMARKER (XCAR (tail)));
+ 	  XSETCAR (tail, Qnil);
+ 	}
+ 
    if (NILP (last_thing_searched))
      return Qnil;
  
***************
*** 2797,2806 ****
  	    /* last_thing_searched must always be Qt, a buffer, or Qnil.  */
  	    abort ();
  
! 	  len = 2*(i+1);
  	}
        else
! 	data[2 * i] = data [2 * i + 1] = Qnil;
      }
  
    if (BUFFERP (last_thing_searched) && !NILP (integers))
--- 2809,2818 ----
  	    /* last_thing_searched must always be Qt, a buffer, or Qnil.  */
  	    abort ();
  
! 	  len = 2 * i + 2;
  	}
        else
! 	data[2 * i] = data[2 * i + 1] = Qnil;
      }
  
    if (BUFFERP (last_thing_searched) && !NILP (integers))
***************
*** 2834,2844 ****
  }
  
  
! DEFUN ("set-match-data", Fset_match_data, Sset_match_data, 1, 1, 0,
         doc: /* Set internal data on last search match from elements of LIST.
! LIST should have been created by calling `match-data' previously.  */)
!      (list)
!      register Lisp_Object list;
  {
    register int i;
    register Lisp_Object marker;
--- 2846,2858 ----
  }
  
  
! DEFUN ("set-match-data", Fset_match_data, Sset_match_data, 1, 2, 0,
         doc: /* Set internal data on last search match from elements of LIST.
! LIST should have been created by calling `match-data' previously.
! 
! If optional arg RESEAT is non-nil, make markers on LIST point nowhere.  */)
!     (list, reseat)
!      register Lisp_Object list, reseat;
  {
    register int i;
    register Lisp_Object marker;
***************
*** 2882,2890 ****
  	search_regs.num_regs = length;
        }
  
!     for (i = 0;; i++)
        {
! 	marker = Fcar (list);
  	if (BUFFERP (marker))
  	  {
  	    last_thing_searched = marker;
--- 2896,2904 ----
  	search_regs.num_regs = length;
        }
  
!     for (i = 0; CONSP (list); i++)
        {
! 	marker = XCAR (list);
  	if (BUFFERP (marker))
  	  {
  	    last_thing_searched = marker;
***************
*** 2895,2906 ****
  	if (NILP (marker))
  	  {
  	    search_regs.start[i] = -1;
! 	    list = Fcdr (list);
  	  }
  	else
  	  {
  	    int from;
  
  	    if (MARKERP (marker))
  	      {
  		if (XMARKER (marker)->buffer == 0)
--- 2909,2922 ----
  	if (NILP (marker))
  	  {
  	    search_regs.start[i] = -1;
! 	    list = XCDR (list);
  	  }
  	else
  	  {
  	    int from;
+ 	    Lisp_Object m;
  
+ 	    m = marker;
  	    if (MARKERP (marker))
  	      {
  		if (XMARKER (marker)->buffer == 0)
***************
*** 2911,2927 ****
  
  	    CHECK_NUMBER_COERCE_MARKER (marker);
  	    from = XINT (marker);
- 	    list = Fcdr (list);
  
! 	    marker = Fcar (list);
  	    if (MARKERP (marker) && XMARKER (marker)->buffer == 0)
  	      XSETFASTINT (marker, 0);
  
  	    CHECK_NUMBER_COERCE_MARKER (marker);
  	    search_regs.start[i] = from;
  	    search_regs.end[i] = XINT (marker);
  	  }
! 	list = Fcdr (list);
        }
  
      for (; i < search_regs.num_regs; i++)
--- 2927,2952 ----
  
  	    CHECK_NUMBER_COERCE_MARKER (marker);
  	    from = XINT (marker);
  
! 	    if (!NILP (reseat) && MARKERP (m))
! 	      unchain_marker (XMARKER (m));
! 
! 	    if ((list = XCDR (list), !CONSP (list)))
! 	      break;
! 
! 	    m = marker = XCAR (list);
! 
  	    if (MARKERP (marker) && XMARKER (marker)->buffer == 0)
  	      XSETFASTINT (marker, 0);
  
  	    CHECK_NUMBER_COERCE_MARKER (marker);
  	    search_regs.start[i] = from;
  	    search_regs.end[i] = XINT (marker);
+ 
+ 	    if (!NILP (reseat) && MARKERP (m))
+ 	      unchain_marker (XMARKER (m));
  	  }
! 	list = XCDR (list);
        }
  
      for (; i < search_regs.num_regs; i++)
***************
*** 2959,2965 ****
  
  /* Called upon exit from filters and sentinels. */
  void
! restore_match_data ()
  {
    if (search_regs_saved)
      {
--- 2984,2990 ----
  
  /* Called upon exit from filters and sentinels. */
  void
! restore_search_regs ()
  {
    if (search_regs_saved)
      {
***************
*** 2977,2982 ****
--- 3002,3022 ----
      }
  }
  
+ static Lisp_Object
+ unwind_set_match_data (list)
+      Lisp_Object list;
+ {
+   return Fset_match_data (list, Qt);
+ }
+ 
+ /* Called to unwind protect the match data.  */
+ void
+ record_unwind_save_match_data ()
+ {
+   record_unwind_protect (unwind_set_match_data,
+ 			 Fmatch_data (Qnil, Qnil, Qnil));
+ }
+ 
  /* Quote a string to inactivate reg-expr chars */
  
  DEFUN ("regexp-quote", Fregexp_quote, Sregexp_quote, 1, 1, 0,
Index: composite.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/composite.c,v
retrieving revision 1.31
diff -c -r1.31 composite.c
*** composite.c	26 Dec 2003 11:39:22 -0000	1.31
--- composite.c	7 Jun 2005 14:28:52 -0000
***************
*** 628,634 ****
      }
  
    /* Preserve the match data.  */
!   record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
  
    /* If none of ASCII characters have composition functions, we can
       skip them quickly.  */
--- 628,634 ----
      }
  
    /* Preserve the match data.  */
!   record_unwind_save_match_data ();
  
    /* If none of ASCII characters have composition functions, we can
       skip them quickly.  */
Index: eval.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/eval.c,v
retrieving revision 1.240
diff -c -r1.240 eval.c
*** eval.c	3 Jun 2005 23:02:21 -0000	1.240
--- eval.c	7 Jun 2005 14:28:52 -0000
***************
*** 1971,1977 ****
    GCPRO3 (fun, funname, fundef);
  
    /* Preserve the match data.  */
!   record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
  
    /* Value saved here is to be restored into Vautoload_queue.  */
    record_unwind_protect (un_autoload, Vautoload_queue);
--- 1971,1977 ----
    GCPRO3 (fun, funname, fundef);
  
    /* Preserve the match data.  */
!   record_unwind_save_match_data ();
  
    /* Value saved here is to be restored into Vautoload_queue.  */
    record_unwind_protect (un_autoload, Vautoload_queue);
Index: macmenu.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/macmenu.c,v
retrieving revision 1.28
diff -c -r1.28 macmenu.c
*** macmenu.c	6 Jun 2005 20:24:13 -0000	1.28
--- macmenu.c	7 Jun 2005 14:28:52 -0000
***************
*** 1475,1481 ****
  	 because it is not reentrant.  */
        specbind (Qdebug_on_next_call, Qnil);
  
!       record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
        if (NILP (Voverriding_local_map_menu_flag))
  	{
  	  specbind (Qoverriding_terminal_local_map, Qnil);
--- 1475,1481 ----
  	 because it is not reentrant.  */
        specbind (Qdebug_on_next_call, Qnil);
  
!       record_unwind_save_match_data ();
        if (NILP (Voverriding_local_map_menu_flag))
  	{
  	  specbind (Qoverriding_terminal_local_map, Qnil);
Index: process.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/process.c,v
retrieving revision 1.455
diff -c -r1.455 process.c
*** process.c	7 Jun 2005 13:19:25 -0000	1.455
--- process.c	7 Jun 2005 14:28:53 -0000
***************
*** 4888,4897 ****
  	{
  	  Lisp_Object tem;
  	  /* Don't clobber the CURRENT match data, either!  */
! 	  tem = Fmatch_data (Qnil, Qnil);
! 	  restore_match_data ();
! 	  record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
! 	  Fset_match_data (tem);
  	}
  
        /* For speed, if a search happens within this code,
--- 4888,4897 ----
  	{
  	  Lisp_Object tem;
  	  /* Don't clobber the CURRENT match data, either!  */
! 	  tem = Fmatch_data (Qnil, Qnil, Qnil);
! 	  restore_search_regs ();
! 	  record_unwind_save_match_data ();
! 	  Fset_match_data (tem, Qt);
  	}
  
        /* For speed, if a search happens within this code,
***************
*** 4945,4951 ****
  				   read_process_output_error_handler);
  
        /* If we saved the match data nonrecursively, restore it now.  */
!       restore_match_data ();
        running_asynch_code = outer_running_asynch_code;
  
        /* Handling the process output should not deactivate the mark.  */
--- 4945,4951 ----
  				   read_process_output_error_handler);
  
        /* If we saved the match data nonrecursively, restore it now.  */
!       restore_search_regs ();
        running_asynch_code = outer_running_asynch_code;
  
        /* Handling the process output should not deactivate the mark.  */
***************
*** 6349,6358 ****
    if (outer_running_asynch_code)
      {
        Lisp_Object tem;
!       tem = Fmatch_data (Qnil, Qnil);
!       restore_match_data ();
!       record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
!       Fset_match_data (tem);
      }
  
    /* For speed, if a search happens within this code,
--- 6349,6358 ----
    if (outer_running_asynch_code)
      {
        Lisp_Object tem;
!       tem = Fmatch_data (Qnil, Qnil, Qnil);
!       restore_search_regs ();
!       record_unwind_save_match_data ();
!       Fset_match_data (tem, Qt);
      }
  
    /* For speed, if a search happens within this code,
***************
*** 6366,6372 ****
  			     exec_sentinel_error_handler);
  
    /* If we saved the match data nonrecursively, restore it now.  */
!   restore_match_data ();
    running_asynch_code = outer_running_asynch_code;
  
    Vdeactivate_mark = odeactivate;
--- 6366,6372 ----
  			     exec_sentinel_error_handler);
  
    /* If we saved the match data nonrecursively, restore it now.  */
!   restore_search_regs ();
    running_asynch_code = outer_running_asynch_code;
  
    Vdeactivate_mark = odeactivate;
Index: w32menu.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/w32menu.c,v
retrieving revision 1.72
diff -c -r1.72 w32menu.c
*** w32menu.c	24 May 2005 08:44:25 -0000	1.72
--- w32menu.c	7 Jun 2005 14:28:54 -0000
***************
*** 1443,1449 ****
  	 because it is not reentrant.  */
        specbind (Qdebug_on_next_call, Qnil);
  
!       record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
        if (NILP (Voverriding_local_map_menu_flag))
  	{
  	  specbind (Qoverriding_terminal_local_map, Qnil);
--- 1443,1450 ----
  	 because it is not reentrant.  */
        specbind (Qdebug_on_next_call, Qnil);
  
!       record_unwind_save_match_data ();
! 
        if (NILP (Voverriding_local_map_menu_flag))
  	{
  	  specbind (Qoverriding_terminal_local_map, Qnil);
Index: xdisp.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/xdisp.c,v
retrieving revision 1.1019
diff -c -r1.1019 xdisp.c
*** xdisp.c	6 Jun 2005 12:36:29 -0000	1.1019
--- xdisp.c	7 Jun 2005 14:28:55 -0000
***************
*** 8458,8464 ****
        Lisp_Object tail, frame;
        int count = SPECPDL_INDEX ();
  
!       record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
  
        FOR_EACH_FRAME (tail, frame)
  	{
--- 8458,8464 ----
        Lisp_Object tail, frame;
        int count = SPECPDL_INDEX ();
  
!       record_unwind_save_match_data ();
  
        FOR_EACH_FRAME (tail, frame)
  	{
***************
*** 8581,8587 ****
  
  	  set_buffer_internal_1 (XBUFFER (w->buffer));
  	  if (save_match_data)
! 	    record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
  	  if (NILP (Voverriding_local_map_menu_flag))
  	    {
  	      specbind (Qoverriding_terminal_local_map, Qnil);
--- 8581,8587 ----
  
  	  set_buffer_internal_1 (XBUFFER (w->buffer));
  	  if (save_match_data)
! 	    record_unwind_save_match_data ();
  	  if (NILP (Voverriding_local_map_menu_flag))
  	    {
  	      specbind (Qoverriding_terminal_local_map, Qnil);
***************
*** 8772,8778 ****
  
  	  /* Save match data, if we must.  */
  	  if (save_match_data)
! 	    record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
  
  	  /* Make sure that we don't accidentally use bogus keymaps.  */
  	  if (NILP (Voverriding_local_map_menu_flag))
--- 8772,8778 ----
  
  	  /* Save match data, if we must.  */
  	  if (save_match_data)
! 	    record_unwind_save_match_data ();
  
  	  /* Make sure that we don't accidentally use bogus keymaps.  */
  	  if (NILP (Voverriding_local_map_menu_flag))
Index: xmenu.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/xmenu.c,v
retrieving revision 1.292
diff -c -r1.292 xmenu.c
*** xmenu.c	6 Jun 2005 12:56:53 -0000	1.292
--- xmenu.c	7 Jun 2005 14:28:55 -0000
***************
*** 2030,2036 ****
  	 because it is not reentrant.  */
        specbind (Qdebug_on_next_call, Qnil);
  
!       record_unwind_protect (Fset_match_data, Fmatch_data (Qnil, Qnil));
        record_unwind_protect (unuse_menu_items, Qnil);
        if (NILP (Voverriding_local_map_menu_flag))
  	{
--- 2030,2036 ----
  	 because it is not reentrant.  */
        specbind (Qdebug_on_next_call, Qnil);
  
!       record_unwind_save_match_data ();
        record_unwind_protect (unuse_menu_items, Qnil);
        if (NILP (Voverriding_local_map_menu_flag))
  	{
cvs diff: I know nothing about config.h
cvs diff: I know nothing about epaths.h
Index: lisp.h
===================================================================
RCS file: /cvsroot/emacs/emacs/src/lisp.h,v
retrieving revision 1.528
diff -c -r1.528 lisp.h
*** lisp.h	24 May 2005 03:44:09 -0000	1.528
--- lisp.h	7 Jun 2005 14:28:55 -0000
***************
*** 2768,2773 ****
--- 2768,2774 ----
  EXFUN (Fbuffer_enable_undo, 1);
  EXFUN (Ferase_buffer, 0);
  extern Lisp_Object Qoverlayp;
+ extern Lisp_Object Qevaporate;
  extern Lisp_Object get_truename_buffer P_ ((Lisp_Object));
  extern struct buffer *all_buffers;
  EXFUN (Fprevious_overlay_change, 1);
***************
*** 2835,2845 ****
  /* defined in search.c */
  extern void shrink_regexp_cache P_ ((void));
  EXFUN (Fstring_match, 3);
! extern void restore_match_data P_ ((void));
! EXFUN (Fmatch_data, 2);
! EXFUN (Fset_match_data, 1);
  EXFUN (Fmatch_beginning, 1);
  EXFUN (Fmatch_end, 1);
  EXFUN (Flooking_at, 1);
  extern int fast_string_match P_ ((Lisp_Object, Lisp_Object));
  extern int fast_c_string_match_ignore_case P_ ((Lisp_Object, const char *));
--- 2836,2847 ----
  /* defined in search.c */
  extern void shrink_regexp_cache P_ ((void));
  EXFUN (Fstring_match, 3);
! extern void restore_search_regs P_ ((void));
! EXFUN (Fmatch_data, 3);
! EXFUN (Fset_match_data, 2);
  EXFUN (Fmatch_beginning, 1);
  EXFUN (Fmatch_end, 1);
+ extern void record_unwind_save_match_data P_ ((void));
  EXFUN (Flooking_at, 1);
  extern int fast_string_match P_ ((Lisp_Object, Lisp_Object));
  extern int fast_c_string_match_ignore_case P_ ((Lisp_Object, const char *));

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Inefficient code in reftex-index.el

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     > There is no other interface into the number of accessible match
>     > strings (which might be nil) rather than
>     > (/ (length (match-data t)) 2).
>
>     That's still pretty inefficient -- I suggest that we introduce a new
>     function `match-count' to return that number.
>
> Is there sufficient use for this function to justify introducing it?
> I think that most cases where this would be used, the code would
> then proceed to call match-data.

I don't know.  

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Inefficient code in reftex-index.el

[Stefan Monnier]

>> There is no other interface into the number of accessible match
>> strings (which might be nil) rather than
>> (/ (length (match-data t)) 2).

>     That's still pretty inefficient -- I suggest that we introduce a new
>     function `match-count' to return that number.

> Is there sufficient use for this function to justify introducing it?
> I think that most cases where this would be used, the code would
> then proceed to call match-data.

Actually the only cases I can vaguely remember using the
(/ (length (match-data)) 2) idiom didn't use the whole (match-data).
They typically used the idiom in order to know *which* subgroup matched (of
course it only works if you craft your regexp carefully).


        Stefan

Re: Inefficient code in reftex-index.el

[Stefan Monnier]

> In any case, to me, the match-data interface should not be considered
> a user-level feature _at all_.  The user level feature is
> save-match-data, which does not (in principle) allow the user to mess
> with the saved data.  And in that respect, nobody should really care
> about what the format of the match data is.

Not sure about `match-data', but `set-match-data' is definitely useful for
cases like font-lock where the MATCHER function is expected to return its
info in the match-data but there may not be any regexp that properly matches
what we're looking for, so the MATCHER function needs to do the match "by
hand" and then manually construct a match data to pass to set-match-data.
See smerge-mode for an example.


        Stefan

Re: Inefficient code in reftex-index.el

[Kim F. Storm]

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

>>> There is no other interface into the number of accessible match
>>> strings (which might be nil) rather than
>>> (/ (length (match-data t)) 2).
>
>>     That's still pretty inefficient -- I suggest that we introduce a new
>>     function `match-count' to return that number.
>
>> Is there sufficient use for this function to justify introducing it?
>> I think that most cases where this would be used, the code would
>> then proceed to call match-data.
>
> Actually the only cases I can vaguely remember using the
> (/ (length (match-data)) 2) idiom didn't use the whole (match-data).
> They typically used the idiom in order to know *which* subgroup matched (of
> course it only works if you craft your regexp carefully).

Maybe this would be more versatile:

(match-subgroups)   => "match count"
(match-subgroups t) => list of subgroups that matches

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Inefficient code in reftex-index.el

[David Kastrup]

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

>>> There is no other interface into the number of accessible match
>>> strings (which might be nil) rather than
>>> (/ (length (match-data t)) 2).
>
>>     That's still pretty inefficient -- I suggest that we introduce a new
>>     function `match-count' to return that number.
>
>> Is there sufficient use for this function to justify introducing it?
>> I think that most cases where this would be used, the code would
>> then proceed to call match-data.
>
> Actually the only cases I can vaguely remember using the
> (/ (length (match-data)) 2) idiom didn't use the whole (match-data).
> They typically used the idiom in order to know *which* subgroup matched (of
> course it only works if you craft your regexp carefully).

That would not work.  (length (match-data)) is a property of the
regexp, not the match.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Inefficient code in reftex-index.el

[Kim F. Storm]

David Kastrup <dak@gnu.org> writes:

>> Actually the only cases I can vaguely remember using the
>> (/ (length (match-data)) 2) idiom didn't use the whole (match-data).
>> They typically used the idiom in order to know *which* subgroup matched (of
>> course it only works if you craft your regexp carefully).
>
> That would not work.  (length (match-data)) is a property of the
> regexp, not the match.

According to the code, it does work.

Match data only contains markers until the last successful match.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Inefficient code in reftex-index.el

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> David Kastrup <dak@gnu.org> writes:
>
>>> Actually the only cases I can vaguely remember using the
>>> (/ (length (match-data)) 2) idiom didn't use the whole (match-data).
>>> They typically used the idiom in order to know *which* subgroup matched (of
>>> course it only works if you craft your regexp carefully).
>>
>> That would not work.  (length (match-data)) is a property of the
>> regexp, not the match.
>
> According to the code, it does work.
>
> Match data only contains markers until the last successful match.

(progn (string-match "\\(x\\)\\|\\(y\\)" "x") (match-data)) => (0 1 0 1)

I stand corrected.

Hey, can I rely on that?  It would probably speed up some code of mine
where I have to scan for the only non-nil match-beginning.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Inefficient code in reftex-index.el

[Richard Stallman]

This `evaporate' idea is a good one, but I agree with the person
who said that the indication should go at the end, not the beginning,
of the return value.  The value returned by `match-data' has
a defined format and this should not be broken.

Re: Inefficient code in reftex-index.el

[Richard Stallman]

    Problem is that markers slow down editing, and significantly so.  And
    normal editing operations are not associated with extensive consing,
    so they won't trigger frequent garbage collection.

That is sufficient reason to urge people to use match-data only when
really necessary, but not a sufficient reason to eliminate match-data.

Re: Inefficient code in reftex-index.el

[Richard Stallman]

    Actually the only cases I can vaguely remember using the
    (/ (length (match-data)) 2) idiom didn't use the whole (match-data).
    They typically used the idiom in order to know *which* subgroup matched (of
    course it only works if you craft your regexp carefully).

Can't you tell that more easily by seeing if match-beginning returns nil?

Re: Inefficient code in reftex-index.el

[David Kastrup]

Richard Stallman <rms@gnu.org> writes:

>     Actually the only cases I can vaguely remember using the (/
>     (length (match-data)) 2) idiom didn't use the whole
>     (match-data).  They typically used the idiom in order to know
>     *which* subgroup matched (of course it only works if you craft
>     your regexp carefully).
>
> Can't you tell that more easily by seeing if match-beginning returns nil?

Which match-beginning?

After (string-match "\\(a\\)\\|\\(b\\)\\|\\(c\\)" input)

I can just consult (length (match-data)) for distinguishing between
all three alternatives.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Inefficient code in reftex-index.el

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

> This `evaporate' idea is a good one, but I agree with the person
> who said that the indication should go at the end, not the beginning,
> of the return value.  The value returned by `match-data' has
> a defined format and this should not be broken.

I have installed the second version of the patch, adding the RESEAT arg to
match-data and set-match-data.

I extended the patch so that specifying `evaporate' for RESEAT will
free the markers immediately.

I changed save-match-data to use evaporate, but the change to replace.el
only passes `t' to match-data -- maybe it could pass evaporate as well.
David?

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Inefficient code in reftex-index.el

[Richard Stallman]

    > Can't you tell that more easily by seeing if match-beginning returns nil?

    Which match-beginning?

One for a subexpression inside the alternative you're trying to test for.

    After (string-match "\\(a\\)\\|\\(b\\)\\|\\(c\\)" input)

    I can just consult (length (match-data)) for distinguishing between
    all three alternatives.

You could, but you'd have to compare the value of that against various
constants, which would be ugly.  I think this code is cleaner:

   (cond ((match-beginning 1)
   	  ...)
	 ((match-beginning 2)
	  ...)
	 ((match-beginning 3)
	  ...)

Re: Inefficient code in reftex-index.el

[David Kastrup]

Richard Stallman <rms@gnu.org> writes:

>     > Can't you tell that more easily by seeing if match-beginning returns nil?
>
>     Which match-beginning?
>
> One for a subexpression inside the alternative you're trying to test for.
>
>     After (string-match "\\(a\\)\\|\\(b\\)\\|\\(c\\)" input)
>
>     I can just consult (length (match-data)) for distinguishing between
>     all three alternatives.
>
> You could, but you'd have to compare the value of that against
> various constants, which would be ugly.

Uh, no.  In the applications I am using, the expression for the string
match is created programmatically from a large list of strings, and
the result is used for indexing into corresponding data structures.

> I think this code is cleaner:
>
>    (cond ((match-beginning 1)
>    	  ...)
> 	 ((match-beginning 2)
> 	  ...)
> 	 ((match-beginning 3)
> 	  ...)

Certainly cleaner than the straw man you are trying to put up, no
question about that.  And I have been using this idiom a number of
times for other code.

In the application that I had in mind, however, no different code
paths were taken, and so this boiled down to

(while (not (match-beginning index))
  (setq index (1+ index)))
(do-something-about index)

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Inefficient code in reftex-index.el

[Richard Stallman]

    Certainly cleaner than the straw man you are trying to put up, no
    question about that.

It's not a straw man.  It's what I'd expect most such cases to do.

If it is very important to optimize your code, we could add
a function match-count to do so.  But is it really that
important to optimize?

Re: Inefficient code in reftex-index.el

[David Kastrup]

Richard Stallman <rms@gnu.org> writes:

>     Certainly cleaner than the straw man you are trying to put up, no
>     question about that.
>
> It's not a straw man.  It's what I'd expect most such cases to do.
>
> If it is very important to optimize your code, we could add a
> function match-count to do so.  But is it really that important to
> optimize?

I think that if one has the situation of a number of strings and
corresponding actions in a data structure, and one wants to repeatedly
search for the next occurence of any matching string, and then do the
recorded corresponding action, this would be a typical situation where
match-count would be convenient.

grepping through the Emacs tree suggests that this usage is not
common, however.  But I did see several occasions where match-count
seemed appropriate as a loop limit for running through possible
matched groups after a match.

There is also the following in tramp.el:
/home/tmp/emacs/lisp/net/tramp.el:6450:  (let* ((nmatches (/ (length (match-data)) 2))
/home/tmp/emacs/lisp/net/tramp.el:6793:    (setq len (/ (length (match-data t)) 2))

In particular the former usage suffers from the "trailing markers"
slowdown.  While it is something that should be fixed here, it would
seem that the availability of match-count would have prevented this
particular abuse.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum