suspicious GCPROs

suspicious GCPROs

[Kenichi Handa]

I've just checked all occurrences of GCPRO and found a few
suspicious places.  Attached is the patch I propose.

The first two patches (fileio.c and keymap.c) are clearly
necessary because GCPROed variables are not yet initialized
when GC occurs.

The last two patchs (xfns.c and xterm.c) are also necessary
for the same reason, but it seems to me that there's no need
of GCPRO in the first place.  What do yo think?

---
Kenichi Handa
handa@m17n.org

Index: ChangeLog
===================================================================
RCS file: /cvsroot/emacs/emacs/src/ChangeLog,v
retrieving revision 1.4950
diff -c -r1.4950 ChangeLog
*** ChangeLog	15 Mar 2006 19:41:35 -0000	1.4950
--- ChangeLog	16 Mar 2006 06:22:38 -0000
***************
*** 1,3 ****
--- 1,15 ----
+ 2006-03-16  Kenichi Handa  <handa@m17n.org>
+ 
+ 	* fileio.c (Fwrite_region): Set visit_file to Qnil before GCPRO
+ 	it.
+ 
+ 	* keymap.c (map_keymap): Set tail to Qnil before GCPRO it.
+ 
+ 	* xfns.c (xg_set_icon): GCPRO `found' after the value is set.
+ 
+ 	* xterm.c (x_term_init): GCPRO `s' and `abs_file' after the values
+ 	are set.
+ 
  2006-03-15  Kim F. Storm  <storm@cua.dk>
  
  	* xdisp.c (extend_face_to_end_of_line): Always add space glyph to
Index: fileio.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/fileio.c,v
retrieving revision 1.562
diff -c -r1.562 fileio.c
*** fileio.c	24 Feb 2006 06:46:44 -0000	1.562
--- fileio.c	16 Mar 2006 06:22:38 -0000
***************
*** 5009,5014 ****
--- 5009,5015 ----
    if (!NILP (start) && !STRINGP (start))
      validate_region (&start, &end);
  
+   visit_file = Qnil;
    GCPRO5 (start, filename, visit, visit_file, lockname);
  
    filename = Fexpand_file_name (filename, Qnil);
Index: keymap.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/keymap.c,v
retrieving revision 1.319
diff -c -r1.319 keymap.c
*** keymap.c	11 Mar 2006 15:25:03 -0000	1.319
--- keymap.c	16 Mar 2006 06:22:38 -0000
***************
*** 699,704 ****
--- 699,705 ----
    struct gcpro gcpro1, gcpro2, gcpro3;
    Lisp_Object tail;
  
+   tail = Qnil;
    GCPRO3 (map, args, tail);
    map = get_keymap (map, 1, autoload);
    for (tail = (CONSP (map) && EQ (Qkeymap, XCAR (map))) ? XCDR (map) : map;
Index: xfns.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/xfns.c,v
retrieving revision 1.663
diff -c -r1.663 xfns.c
*** xfns.c	12 Mar 2006 01:37:33 -0000	1.663
--- xfns.c	16 Mar 2006 06:22:39 -0000
***************
*** 792,801 ****
    int result = 0;
    Lisp_Object found;
  
-   GCPRO1 (found);
- 
    found = x_find_image_file (file);
  
    if (! NILP (found))
      {
        GdkPixbuf *pixbuf;
--- 792,800 ----
    int result = 0;
    Lisp_Object found;
  
    found = x_find_image_file (file);
  
+   GCPRO1 (found);
    if (! NILP (found))
      {
        GdkPixbuf *pixbuf;
Index: xterm.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/xterm.c,v
retrieving revision 1.903
diff -c -r1.903 xterm.c
*** xterm.c	14 Mar 2006 15:38:43 -0000	1.903
--- xterm.c	16 Mar 2006 06:22:39 -0000
***************
*** 10189,10196 ****
            char *file = "~/.emacs.d/gtkrc";
            Lisp_Object s, abs_file;
  
-           GCPRO2 (s, abs_file);
            s = make_string (file, strlen (file));
            abs_file = Fexpand_file_name (s, Qnil);
  
            if (! NILP (abs_file) && !NILP (Ffile_readable_p (abs_file)))
--- 10189,10197 ----
            char *file = "~/.emacs.d/gtkrc";
            Lisp_Object s, abs_file;
  
            s = make_string (file, strlen (file));
+           abs_file = Qnil;
+           GCPRO2 (s, abs_file);
            abs_file = Fexpand_file_name (s, Qnil);
  
            if (! NILP (abs_file) && !NILP (Ffile_readable_p (abs_file)))

Re: suspicious GCPROs

[Richard Stallman]

I agree that those two gcpros in xterm.c and xfns.c
are unnecessary.

Re: suspicious GCPROs

[Kenichi Handa]

In article <E1FKHsJ-0000OG-N2@fencepost.gnu.org>, Richard Stallman <rms@gnu.org> writes:

> I agree that those two gcpros in xterm.c and xfns.c
> are unnecessary.

Ok, I've just installed these changes.

2006-03-20  Kenichi Handa  <handa@m17n.org>

	* fileio.c (Fwrite_region): Set visit_file to Qnil before GCPRO
	it.

	* keymap.c (map_keymap): Set tail to Qnil before GCPRO it.

	* xfns.c (xg_set_icon): Remove unnecessary GCPRO.

	* xterm.c (x_term_init): Remove unnecessary GCPRO.

---
Kenichi Handa
handa@m17n.org

Re: suspicious GCPROs

[Stefan Monnier]

> I've just checked all occurrences of GCPRO and found a few
> suspicious places.  Attached is the patch I propose.

> The first two patches (fileio.c and keymap.c) are clearly
> necessary because GCPROed variables are not yet initialized
> when GC occurs.

> The last two patchs (xfns.c and xterm.c) are also necessary
> for the same reason, but it seems to me that there's no need
> of GCPRO in the first place.  What do yo think?

Looks good to me, thanks,


        Stefan