emacs' turn: remove useless if-before-free tests

emacs' turn: remove useless if-before-free tests

[Jim Meyering]

Hello,

I propose to remove all useless if-before-free tests from emacs' C code.
When I've done this for other projects, the first reaction is that
it might result in incorrect code on some system people care about.
SunOS4 is no longer supported or even used, so that's not an issue.
On the non-Linux front, cygwin and mingw have had POSIX-compliant free
functions for a long time.  There really is no problem anymore.

Removing such tests wasn't a problem back in 2006:
  http://www.winehq.org/pipermail/wine-patches/2006-October/031544.html
and is even less of a problem now.

So far, I've done this to at least the following
packages with no adverse reports:
    gnulib
    coreutils
    git
    glibc
    freeIPA
    libvirt

The only other argument against this sort of change has been that it
*might* result in some slight (on the order of a few instructions)
performance degradation in some unusual circumstances.  To that, I say
"measure first, _then_ optimize, if warranted".  Besides, if you're
calling "free(NULL)" in a hot loop, I suspect that the few instructions
that you'd save by avoiding the library call will not make a significant
difference.  And even if there is a measurable difference, it is easily
trumped by the improvement in readability/maintainability of removing
the useless "if".

Here are three change sets:
(notice that there are/were two xfree function definitions.
 c-set #2 removes the useless one in ebrowse.c)

	Make "xfree (NULL)" a no-op; remove useless if-before-xfree.
	* alloc.c (xfree): Return right away for a NULL arg.
	* lread.c (nosuffix): Remove now-useless if-before-xfree tests.
	* gtkutil.c (xg_gtk_scroll_destroy): Likewise.
	* mac.c (create_apple_event_from_event_ref): Likewise.
	(create_apple_event_from_drag_ref, cfstring_create_normalized): Likewise.
	* doprnt.c (doprnt1): Likewise.
	* frame.c (frame): Likewise.
	* keyboard.c (wipe_kboard): Likewise.
	* macterm.c (x_free_frame_resources, xlfdpat_destroy, XFreePixmap):
	(init_font_name_table, mac_unload_font, x_delete_display): Likewise.
	* term.c (tty_default_color_capabilities, maybe_fatal)
	(delete_tty): Likewise.
	* w16select.c (string): Likewise.
	* w32.c (w32_get_resource, SET_ENV_BUF_SIZE): Likewise.
	* w32bdf.c (w32_free_bdf_font): Likewise.
	* w32fns.c (w32_unload_font): Likewise.
	* w32font.c (w32font_close): Likewise.
	* window.c (size_window): Likewise.
	* xselect.c (receive_incremental_selection): Likewise.
	* xterm.c (x_free_frame_resources, x_delete_display): Likewise.
	* src/mactoolbox.c (create_apple_event_from_drag_ref): Likewise.
	* src/w32.c (stat): Likewise.

	* ebrowse.c (xfree): Remove definition; s/xfree/free/

	remove useless if-before-free tests
	* lib-src/ebrowse.c (xfree): Likewise.
	* lib-src/etags.c (process_file_name, free_tree, free_fdesc): Likewise.
	(popclass_above, Prolog_functions, Erlang_functions): Likewise.
	* lib-src/pop.c (pop_quit): Likewise.
	* lwlib/lwlib-Xm.c (xm_update_one_value): Likewise.
	* lwlib/lwlib.c (safe_free_str, free_widget_value_tree): Likewise.
	* src/editfns.c (Fset_time_zone_rule): Likewise.
	* src/lread.c (nosuffix): Likewise.
	* src/ralloc.c (get_bloc): Likewise.
	* src/regex.c (reg_free): Likewise.
	* src/xftfont.c (xftfont_open, xftfont_close): Likewise.
	* src/xrdb.c (get_user_app, get_environ_db, x_load_resources): Likewise.
	* src/xsmfns.c (smc_save_yourself_CB): Likewise.

If you approve, I'll be happy to commit them.

FYI, I've been using these changes (rebased regularly)
in git snapshots built every few days for the last 2-3 months.

Also, I've performed most of these changes mechanically,
using code like this (this just detects):
  http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=blob_plain;f=build-aux/useless-if-before-free

Jim
----------------------------------

From 544492d9f07c390322ab178d7318d06640c3912a Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Sat, 16 Feb 2008 14:20:58 +0100
Subject: [PATCH] remove useless if-before-free tests

* lib-src/ebrowse.c (xfree): Likewise.
* lib-src/etags.c (process_file_name, free_tree, free_fdesc): Likewise.
(popclass_above, Prolog_functions, Erlang_functions): Likewise.
* lib-src/pop.c (pop_quit): Likewise.
* lwlib/lwlib-Xm.c (xm_update_one_value): Likewise.
* lwlib/lwlib.c (safe_free_str, free_widget_value_tree): Likewise.
* src/editfns.c (Fset_time_zone_rule): Likewise.
* src/lread.c (nosuffix): Likewise.
* src/ralloc.c (get_bloc): Likewise.
* src/regex.c (reg_free): Likewise.
* src/xftfont.c (xftfont_open, xftfont_close): Likewise.
* src/xrdb.c (get_user_app, get_environ_db, x_load_resources): Likewise.
* src/xsmfns.c (smc_save_yourself_CB): Likewise.
---
 lib-src/ebrowse.c |    3 +--
 lib-src/etags.c   |   26 +++++++++++---------------
 lib-src/pop.c     |    3 +--
 lwlib/lwlib-Xm.c  |    9 +++------
 lwlib/lwlib.c     |    8 ++++----
 src/editfns.c     |    3 +--
 src/lread.c       |    3 +--
 src/ralloc.c      |    3 +--
 src/regex.c       |    9 +++------
 src/xrdb.c        |   13 +++++--------
 src/xsmfns.c      |    3 +--
 11 files changed, 32 insertions(+), 51 deletions(-)

diff --git a/lib-src/ebrowse.c b/lib-src/ebrowse.c
index 3cc85a7..faa5be0 100644
--- a/lib-src/ebrowse.c
+++ b/lib-src/ebrowse.c
@@ -595,8 +595,7 @@ void
 xfree (p)
      void *p;
 {
-  if (p)
-    free (p);
+  free (p);
 }


diff --git a/lib-src/etags.c b/lib-src/etags.c
index 675e926..23eefd6 100644
--- a/lib-src/etags.c
+++ b/lib-src/etags.c
@@ -1799,8 +1799,8 @@ process_file_name (file, lang)
     pfatal (file);

  cleanup:
-  if (compressed_name) free (compressed_name);
-  if (uncompressed_name) free (uncompressed_name);
+  free (compressed_name);
+  free (uncompressed_name);
   last_node = NULL;
   curfdp = NULL;
   return;
@@ -2179,8 +2179,7 @@ free_tree (np)
     {
       register node *node_right = np->right;
       free_tree (np->left);
-      if (np->name != NULL)
-	free (np->name);
+      free (np->name);
       free (np->regex);
       free (np);
       np = node_right;
@@ -2195,11 +2194,11 @@ static void
 free_fdesc (fdp)
      register fdesc *fdp;
 {
-  if (fdp->infname != NULL) free (fdp->infname);
-  if (fdp->infabsname != NULL) free (fdp->infabsname);
-  if (fdp->infabsdir != NULL) free (fdp->infabsdir);
-  if (fdp->taggedfname != NULL) free (fdp->taggedfname);
-  if (fdp->prop != NULL) free (fdp->prop);
+  free (fdp->infname);
+  free (fdp->infabsname);
+  free (fdp->infabsdir);
+  free (fdp->taggedfname);
+  free (fdp->prop);
   free (fdp);
 }

@@ -2844,8 +2843,7 @@ popclass_above (bracelev)
        nl >= 0 && cstack.bracelev[nl] >= bracelev;
        nl--)
     {
-      if (cstack.cname[nl] != NULL)
-	free (cstack.cname[nl]);
+      free (cstack.cname[nl]);
       cstack.nl = nl;
     }
 }
@@ -5521,8 +5519,7 @@ Prolog_functions (inf)
 	  last[len] = '\0';
 	}
     }
-  if (last != NULL)
-    free (last);
+  free (last);
 }


@@ -5700,8 +5697,7 @@ Erlang_functions (inf)
 	  last[len] = '\0';
 	}
     }
-  if (last != NULL)
-    free (last);
+  free (last);
 }


diff --git a/lib-src/pop.c b/lib-src/pop.c
index 96be6af..76cd2fb 100644
--- a/lib-src/pop.c
+++ b/lib-src/pop.c
@@ -998,8 +998,7 @@ pop_quit (server)
       close (server->file);
     }

-  if (server->buffer)
-    free (server->buffer);
+  free (server->buffer);
   free ((char *) server);

   return (ret);
diff --git a/lwlib/lwlib-Xm.c b/lwlib/lwlib-Xm.c
index 83df050..7c91fe8 100644
--- a/lwlib/lwlib-Xm.c
+++ b/lwlib/lwlib-Xm.c
@@ -956,15 +956,13 @@ xm_update_one_value (instance, widget, val)
     }
   else if (class == xmTextWidgetClass)
     {
-      if (val->value)
-	free (val->value);
+      free (val->value);
       val->value = XmTextGetString (widget);
       val->edited = True;
     }
   else if (class == xmTextFieldWidgetClass)
     {
-      if (val->value)
-	free (val->value);
+      free (val->value);
       val->value = XmTextFieldGetString (widget);
       val->edited = True;
     }
@@ -989,8 +987,7 @@ xm_update_one_value (instance, widget, val)
 	      XtVaGetValues (toggle, XmNset, &set, NULL);
 	      if (set)
 		{
-		  if (val->value)
-		    free (val->value);
+		  free (val->value);
 		  val->value = safe_strdup (XtName (toggle));
 		}
 	    }
diff --git a/lwlib/lwlib.c b/lwlib/lwlib.c
index 7b2752b..03c4021 100644
--- a/lwlib/lwlib.c
+++ b/lwlib/lwlib.c
@@ -170,7 +170,7 @@ static void
 safe_free_str (s)
      char *s;
 {
-  if (s) free (s);
+  free (s);
 }

 static widget_value *widget_value_free_list = 0;
@@ -226,9 +226,9 @@ free_widget_value_tree (wv)
   if (!wv)
     return;

-  if (wv->name) free (wv->name);
-  if (wv->value) free (wv->value);
-  if (wv->key) free (wv->key);
+  free (wv->name);
+  free (wv->value);
+  free (wv->key);

   wv->name = wv->value = wv->key = (char *) 0xDEADBEEF;

diff --git a/src/editfns.c b/src/editfns.c
index 95ad2f1..192277e 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -2038,8 +2038,7 @@ If TZ is t, use Universal Time.  */)
     }

   set_time_zone_rule (tzstring);
-  if (environbuf)
-    free (environbuf);
+  free (environbuf);
   environbuf = environ;

   return Qnil;
diff --git a/src/lread.c b/src/lread.c
index 1bc9ec3..578addf 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -1269,8 +1269,7 @@ Return t if the file exists and loads successfully.  */)

   UNGCPRO;

-  if (saved_doc_string)
-    free (saved_doc_string);
+  free (saved_doc_string);
   saved_doc_string = 0;
   saved_doc_string_size = 0;

diff --git a/src/ralloc.c b/src/ralloc.c
index 652cf30..1c98828 100644
--- a/src/ralloc.c
+++ b/src/ralloc.c
@@ -425,8 +425,7 @@ get_bloc (size)
   if (! (new_bloc = (bloc_ptr) malloc (BLOC_PTR_SIZE))
       || ! (new_bloc->data = obtain (break_value, size)))
     {
-      if (new_bloc)
-	free (new_bloc);
+      free (new_bloc);

       return 0;
     }
diff --git a/src/regex.c b/src/regex.c
index cbc6756..8ffd2be 100644
--- a/src/regex.c
+++ b/src/regex.c
@@ -6841,20 +6841,17 @@ void
 regfree (preg)
     regex_t *preg;
 {
-  if (preg->buffer != NULL)
-    free (preg->buffer);
+  free (preg->buffer);
   preg->buffer = NULL;

   preg->allocated = 0;
   preg->used = 0;

-  if (preg->fastmap != NULL)
-    free (preg->fastmap);
+  free (preg->fastmap);
   preg->fastmap = NULL;
   preg->fastmap_accurate = 0;

-  if (preg->translate != NULL)
-    free (preg->translate);
+  free (preg->translate);
   preg->translate = NULL;
 }
 WEAK_ALIAS (__regfree, regfree)
diff --git a/src/xrdb.c b/src/xrdb.c
index e199c60..961f274 100644
--- a/src/xrdb.c
+++ b/src/xrdb.c
@@ -431,13 +431,11 @@ get_user_app (class)
     {
       XrmDatabase db = XrmGetFileDatabase (file);
       free (file);
-      if (free_it)
-	free (free_it);
+      free (free_it);
       return db;
     }

-  if (free_it)
-    free (free_it);
+  free (free_it);
   return NULL;
 }

@@ -504,8 +502,8 @@ get_environ_db ()

   db = XrmGetFileDatabase (p);

-  if (path) free (path);
-  if (home) free (home);
+  free (path);
+  free (home);

   return db;
 }
@@ -612,8 +610,7 @@ x_load_resources (display, xrm_string, myname, myclass)

   /* Figure out what the "customization string" is, so we can use it
      to decode paths.  */
-  if (x_customization_string)
-    free (x_customization_string);
+  free (x_customization_string);
   x_customization_string
     = x_get_customization_string (user_database, myname, myclass);

diff --git a/src/xsmfns.c b/src/xsmfns.c
index 538698a..ecbf259 100644
--- a/src/xsmfns.c
+++ b/src/xsmfns.c
@@ -262,8 +262,7 @@ smc_save_yourself_CB (smcConn,

   xfree (smid_opt);

-  if (cwd)
-    free (cwd);
+  free (cwd);

   /* See if we maybe shall interact with the user.  */
   if (interactStyle != SmInteractStyleAny
--
1.5.6.rc0.47.g35cb


From 30cbcc11e58214476b2843b89240160877952a41 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Sat, 16 Feb 2008 15:29:28 +0100
Subject: [PATCH] step 2: ebrowse.c (xfree): Remove definition; s/xfree/free/

---
 lib-src/ebrowse.c |   24 +++++++-----------------
 1 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/lib-src/ebrowse.c b/lib-src/ebrowse.c
index faa5be0..720e020 100644
--- a/lib-src/ebrowse.c
+++ b/lib-src/ebrowse.c
@@ -589,16 +589,6 @@ xrealloc (p, sz)
 }


-/* Like free but always check for null pointers..  */
-
-void
-xfree (p)
-     void *p;
-{
-  free (p);
-}
-
-
 /* Like strdup, but print an error and exit if not enough memory is
    available..  If S is null, return null.  */

@@ -2758,7 +2748,7 @@ member (cls, vis)
           if (LOOKING_AT ('{') && id && cls)
 	    add_member_defn (cls, id, regexp, pos, hash, 0, sc, flags);

-	  xfree (id);
+	  free (id);
           id = NULL;
           sc = SC_MEMBER;
           break;
@@ -2837,7 +2827,7 @@ member (cls, vis)
       print_info ();
     }

-  xfree (id);
+  free (id);
 }


@@ -3074,7 +3064,7 @@ parse_qualified_ident_or_type (last_id)
 	    cls = add_sym (id, cls);

 	  *last_id = NULL;
-	  xfree (id);
+	  free (id);
 	  id = NULL;
 	  id_size = 0;
 	  MATCH ();
@@ -3277,7 +3267,7 @@ add_declarator (cls, id, flags, sc)
       print_info ();
     }

-  xfree (*id);
+  free (*id);
   *id = NULL;
   *cls = NULL;
 }
@@ -3330,7 +3320,7 @@ declaration (flags)
              `declare (X, Y)\n class A : ...'.  */
           if (id)
 	    {
-	      xfree (id);
+	      free (id);
 	      return;
 	    }

@@ -3424,7 +3414,7 @@ declaration (flags)
           if (!cls && id && LOOKING_AT ('{'))
 	    add_global_defn (id, regexp, pos, hash, 0, sc, flags);

-	  xfree (id);
+	  free (id);
           id = NULL;
           break;
         }
@@ -3480,7 +3470,7 @@ globals (start_flags)
                     MATCH_IF ('}');
                   }

-		xfree (namespace_name);
+		free (namespace_name);
               }
           }
           break;
--
1.5.6.rc0.47.g35cb


From 6a9a7661bc768c49c9d728b48a29683129d1d14e Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Sat, 8 Mar 2008 16:27:54 +0100
Subject: [PATCH] Make "xfree (NULL)" a no-op; remove useless if-before-xfree.

* alloc.c (xfree): Return right away for a NULL arg.
* lread.c (nosuffix): Remove now-useless if-before-xfree tests.
* gtkutil.c (xg_gtk_scroll_destroy): Likewise.
* mac.c (create_apple_event_from_event_ref): Likewise.
(create_apple_event_from_drag_ref, cfstring_create_normalized): Likewise.
* doprnt.c (doprnt1): Likewise.
* frame.c (frame): Likewise.
* keyboard.c (wipe_kboard): Likewise.
* macterm.c (x_free_frame_resources, xlfdpat_destroy, XFreePixmap):
(init_font_name_table, mac_unload_font, x_delete_display): Likewise.
* term.c (tty_default_color_capabilities, maybe_fatal)
(delete_tty): Likewise.
* w16select.c (string): Likewise.
* w32.c (w32_get_resource, SET_ENV_BUF_SIZE): Likewise.
* w32bdf.c (w32_free_bdf_font): Likewise.
* w32fns.c (w32_unload_font): Likewise.
* w32font.c (w32font_close): Likewise.
* window.c (size_window): Likewise.
* xselect.c (receive_incremental_selection): Likewise.
* xterm.c (x_free_frame_resources, x_delete_display): Likewise.
* src/mactoolbox.c (create_apple_event_from_drag_ref): Likewise.
* src/w32.c (stat): Likewise.
---
 src/alloc.c      |    2 ++
 src/doprnt.c     |    3 +--
 src/frame.c      |   21 +++++++--------------
 src/gtkutil.c    |    2 +-
 src/keyboard.c   |    3 +--
 src/lread.c      |    3 +--
 src/mac.c        |    9 +++------
 src/macterm.c    |   27 +++++++++------------------
 src/mactoolbox.c |    3 +--
 src/term.c       |   26 ++++++++------------------
 src/w16select.c  |    3 +--
 src/w32.c        |    9 ++++-----
 src/w32bdf.c     |    8 ++++----
 src/w32fns.c     |    2 +-
 src/window.c     |    2 +-
 src/xselect.c    |    2 +-
 src/xterm.c      |   10 +++-------
 17 files changed, 49 insertions(+), 86 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 031cb99..797b385 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -797,6 +797,8 @@ void
 xfree (block)
      POINTER_TYPE *block;
 {
+  if (!block)
+    return;
   MALLOC_BLOCK_INPUT;
   free (block);
   MALLOC_UNBLOCK_INPUT;
diff --git a/src/doprnt.c b/src/doprnt.c
index 05b194c..c7c27ba 100644
--- a/src/doprnt.c
+++ b/src/doprnt.c
@@ -326,8 +326,7 @@ doprnt1 (lispstrings, buffer, bufsize, format, format_end, nargs, args)
     };

   /* If we had to malloc something, free it.  */
-  if (big_buffer)
-    xfree (big_buffer);
+  xfree (big_buffer);

   *bufptr = 0;		/* Make sure our string end with a '\0' */
   return bufptr - buffer;
diff --git a/src/frame.c b/src/frame.c
index 2081287..84a4e54 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -1477,20 +1477,13 @@ But FORCE inhibits this too.  */)
   Vframe_list = Fdelq (frame, Vframe_list);
   FRAME_SET_VISIBLE (f, 0);

-  if (f->namebuf)
-    xfree (f->namebuf);
-  if (f->decode_mode_spec_buffer)
-    xfree (f->decode_mode_spec_buffer);
-  if (FRAME_INSERT_COST (f))
-    xfree (FRAME_INSERT_COST (f));
-  if (FRAME_DELETEN_COST (f))
-    xfree (FRAME_DELETEN_COST (f));
-  if (FRAME_INSERTN_COST (f))
-    xfree (FRAME_INSERTN_COST (f));
-  if (FRAME_DELETE_COST (f))
-    xfree (FRAME_DELETE_COST (f));
-  if (FRAME_MESSAGE_BUF (f))
-    xfree (FRAME_MESSAGE_BUF (f));
+  xfree (f->namebuf);
+  xfree (f->decode_mode_spec_buffer);
+  xfree (FRAME_INSERT_COST (f));
+  xfree (FRAME_DELETEN_COST (f));
+  xfree (FRAME_INSERTN_COST (f));
+  xfree (FRAME_DELETE_COST (f));
+  xfree (FRAME_MESSAGE_BUF (f));

   /* Since some events are handled at the interrupt level, we may get
      an event for f at any time; if we zero out the frame's terminal
diff --git a/src/gtkutil.c b/src/gtkutil.c
index fd0fbf7..afafee1 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -3106,7 +3106,7 @@ xg_gtk_scroll_destroy (widget, data)
   int id = (int) (EMACS_INT) data; /* The EMACS_INT cast avoids a warning. */

   p = g_object_get_data (G_OBJECT (widget), XG_LAST_SB_DATA);
-  if (p) xfree (p);
+  xfree (p);
   xg_remove_widget_from_map (id);
 }

diff --git a/src/keyboard.c b/src/keyboard.c
index 45f3d2f..5ac28a5 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -11575,8 +11575,7 @@ static void
 wipe_kboard (kb)
      KBOARD *kb;
 {
-  if (kb->kbd_macro_buffer)
-    xfree (kb->kbd_macro_buffer);
+  xfree (kb->kbd_macro_buffer);
 }

 #ifdef MULTI_KBOARD
diff --git a/src/lread.c b/src/lread.c
index 578addf..e5e77bc 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -1273,8 +1273,7 @@ Return t if the file exists and loads successfully.  */)
   saved_doc_string = 0;
   saved_doc_string_size = 0;

-  if (prev_saved_doc_string)
-    xfree (prev_saved_doc_string);
+  xfree (prev_saved_doc_string);
   prev_saved_doc_string = 0;
   prev_saved_doc_string_size = 0;

diff --git a/src/mac.c b/src/mac.c
index e549524..20872b2 100644
--- a/src/mac.c
+++ b/src/mac.c
@@ -906,8 +906,7 @@ mac_event_parameters_to_lisp (event, num_params, names, types)
 	  break;
 	}
     }
-  if (buf)
-    xfree (buf);
+  xfree (buf);

   return result;
 }
@@ -4778,10 +4777,8 @@ cfstring_create_normalized (str, symbol)
 					       out_len / sizeof (UniChar));
       if (uni)
 	DisposeUnicodeToTextInfo (&uni);
-      if (out_buf)
-	xfree (out_buf);
-      if (buffer)
-	xfree (buffer);
+      xfree (out_buf);
+      xfree (buffer);
     }
   else
     {
diff --git a/src/macterm.c b/src/macterm.c
index ae9aa11..2871bfa 100644
--- a/src/macterm.c
+++ b/src/macterm.c
@@ -910,8 +910,7 @@ XFreePixmap (display, pixmap)
 #if USE_MAC_IMAGE_IO
   if (pixmap)
     {
-      if (pixmap->data)
-	xfree (pixmap->data);
+      xfree (pixmap->data);
       xfree (pixmap);
     }
 #else
@@ -7390,8 +7389,7 @@ x_free_frame_resources (f)

   x_free_gcs (f);

-  if (FRAME_SIZE_HINTS (f))
-    xfree (FRAME_SIZE_HINTS (f));
+  xfree (FRAME_SIZE_HINTS (f));

   xfree (f->output_data.mac);
   f->output_data.mac = NULL;
@@ -7640,8 +7638,7 @@ xlfdpat_destroy (pat)
     {
       if (pat->buf)
 	{
-	  if (pat->blocks)
-	    xfree (pat->blocks);
+	  xfree (pat->blocks);
 	  xfree (pat->buf);
 	}
       xfree (pat);
@@ -8364,8 +8361,7 @@ init_font_name_table ()
 					 HASH_VALUE (h, j));
 	    prev_family = family;
 	  }
-      if (font_ids)
-	xfree (font_ids);
+      xfree (font_ids);
     }
 #endif

@@ -9242,20 +9238,17 @@ mac_unload_font (dpyinfo, font)
       int i;

       for (i = font->min_byte1; i <= font->max_byte1; i++)
-	if (font->bounds.rows[i])
-	  xfree (font->bounds.rows[i]);
+	xfree (font->bounds.rows[i]);
       xfree (font->bounds.rows);
       ATSUDisposeStyle (font->mac_style);
     }
   else
 #endif
-    if (font->bounds.per_char)
-      xfree (font->bounds.per_char);
+    xfree (font->bounds.per_char);
 #if USE_CG_TEXT_DRAWING
   if (font->cg_font)
     CGFontRelease (font->cg_font);
-  if (font->cg_glyphs)
-    xfree (font->cg_glyphs);
+  xfree (font->cg_glyphs);
 #endif
   xfree (font);
 }
@@ -13026,12 +13019,10 @@ x_delete_display (dpyinfo)

   if (dpyinfo->font_table)
     {
-      if (dpyinfo->font_table->font_encoder)
-	xfree (dpyinfo->font_table->font_encoder);
+      xfree (dpyinfo->font_table->font_encoder);
       xfree (dpyinfo->font_table);
     }
-  if (dpyinfo->mac_id_name)
-    xfree (dpyinfo->mac_id_name);
+  xfree (dpyinfo->mac_id_name);

   if (x_display_list == 0)
     {
diff --git a/src/mactoolbox.c b/src/mactoolbox.c
index b5e87a3..a73578d 100644
--- a/src/mactoolbox.c
+++ b/src/mactoolbox.c
@@ -6091,8 +6091,7 @@ create_apple_event_from_drag_ref (drag, num_types, types, result)
       if (err != noErr)
 	break;
     }
-  if (buf)
-    xfree (buf);
+  xfree (buf);

   if (err == noErr)
     {
diff --git a/src/term.c b/src/term.c
index 7636f22..533104d 100644
--- a/src/term.c
+++ b/src/term.c
@@ -2090,17 +2090,14 @@ tty_default_color_capabilities (struct tty_display_info *tty, int save)

   if (save)
     {
-      if (default_orig_pair)
-	xfree (default_orig_pair);
+      xfree (default_orig_pair);
       default_orig_pair = tty->TS_orig_pair ? xstrdup (tty->TS_orig_pair) : NULL;

-      if (default_set_foreground)
-	xfree (default_set_foreground);
+      xfree (default_set_foreground);
       default_set_foreground = tty->TS_set_foreground ? xstrdup (tty->TS_set_foreground)
 			       : NULL;

-      if (default_set_background)
-	xfree (default_set_background);
+      xfree (default_set_background);
       default_set_background = tty->TS_set_background ? xstrdup (tty->TS_set_background)
 			       : NULL;

@@ -3833,8 +3830,7 @@ maybe_fatal (must_succeed, buffer, terminal, str1, str2, arg1, arg2)
      struct terminal *terminal;
      char *str1, *str2, *arg1, *arg2;
 {
-  if (buffer)
-    xfree (buffer);
+  xfree (buffer);

   if (terminal)
     delete_tty (terminal);
@@ -3915,11 +3911,8 @@ delete_tty (struct terminal *terminal)

   delete_terminal (terminal);

-  if (tty->name)
-    xfree (tty->name);
-
-  if (tty->type)
-    xfree (tty->type);
+  xfree (tty->name);
+  xfree (tty->type);

   if (tty->input)
     {
@@ -3932,11 +3925,8 @@ delete_tty (struct terminal *terminal)
   if (tty->termscript)
     fclose (tty->termscript);

-  if (tty->old_tty)
-    xfree (tty->old_tty);
-
-  if (tty->Wcm)
-    xfree (tty->Wcm);
+  xfree (tty->old_tty);
+  xfree (tty->Wcm);

   bzero (tty, sizeof (struct tty_display_info));
   xfree (tty);
diff --git a/src/w16select.c b/src/w16select.c
index 24e92b2..177a84f 100644
--- a/src/w16select.c
+++ b/src/w16select.c
@@ -565,8 +565,7 @@ DEFUN ("w16-set-clipboard-data", Fw16_set_clipboard_data, Sw16_set_clipboard_dat
   ok = 0;

  unblock:
-  if (dst)
-    xfree (dst);
+  xfree (dst);
   UNBLOCK_INPUT;

   /* Notify user if the text is too large to fit into DOS memory.
diff --git a/src/w32.c b/src/w32.c
index 36635a4..81accb6 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -1128,7 +1128,7 @@ w32_get_resource (key, lpdwtype)
 	  return (lpvalue);
 	}

-      if (lpvalue) xfree (lpvalue);
+      xfree (lpvalue);

       RegCloseKey (hrootkey);
     }
@@ -1145,7 +1145,7 @@ w32_get_resource (key, lpdwtype)
 	  return (lpvalue);
 	}

-      if (lpvalue) xfree (lpvalue);
+      xfree (lpvalue);

       RegCloseKey (hrootkey);
     }
@@ -1346,7 +1346,7 @@ init_environment (char ** argv)
 		/* Also ignore empty environment variables.  */
 		|| *lpval == 0)
 	      {
-		if (lpval) xfree (lpval);
+		xfree (lpval);
 		lpval = env_vars[i].def_value;
 		dwType = REG_EXPAND_SZ;
 		dont_free = 1;
@@ -2962,8 +2962,7 @@ stat (const char * path, struct stat * buf)

       get_file_owner_and_group (NULL, name, buf);
     }
-  if (psd)
-    xfree (psd);
+  xfree (psd);

 #if 0
   /* Not sure if there is any point in this.  */
diff --git a/src/w32bdf.c b/src/w32bdf.c
index e7fbe51..e9a1e63 100644
--- a/src/w32bdf.c
+++ b/src/w32bdf.c
@@ -304,10 +304,10 @@ w32_free_bdf_font(bdffont *fontp)
   CloseHandle(fontp->hfilemap);
   CloseHandle(fontp->hfile);

-  if (fontp->registry) xfree(fontp->registry);
-  if (fontp->encoding) xfree(fontp->encoding);
-  if (fontp->slant) xfree(fontp->slant);
-/*  if (fontp->width) xfree(fontp->width); */
+  xfree(fontp->registry);
+  xfree(fontp->encoding);
+  xfree(fontp->slant);
+/*  xfree(fontp->width); */

   xfree(fontp->filename);
   for(i = 0;i < BDF_FIRST_OFFSET_TABLE;i++)
diff --git a/src/w32fns.c b/src/w32fns.c
index b18a123..e83acf0 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -4955,7 +4955,7 @@ w32_unload_font (dpyinfo, font)
 {
   if (font)
     {
-      if (font->per_char) xfree (font->per_char);
+      xfree (font->per_char);
       if (font->bdf) w32_free_bdf_font (font->bdf);

       if (font->hfont) DeleteObject (font->hfont);
diff --git a/src/window.c b/src/window.c
index c5e8b56..295e45b 100644
--- a/src/window.c
+++ b/src/window.c
@@ -3195,7 +3195,7 @@ size_window (window, size, width_p, nodelete_p, first_only, last_only)
           last_pos += new_size;
 	}

-      if (new_sizes) xfree (new_sizes);
+      xfree (new_sizes);

       /* We should have covered the parent exactly with child windows.  */
       xassert (size == last_pos - first_pos);
diff --git a/src/xselect.c b/src/xselect.c
index dd5c1dc..11ee594 100644
--- a/src/xselect.c
+++ b/src/xselect.c
@@ -1673,7 +1673,7 @@ receive_incremental_selection (display, window, property, target_type,
 	    XSelectInput (display, window, STANDARD_EVENT_SET);
 	  /* Use xfree, not XFree, because x_get_window_property
 	     calls xmalloc itself.  */
-	  if (tmp_data) xfree (tmp_data);
+	  xfree (tmp_data);
 	  break;
 	}

diff --git a/src/xterm.c b/src/xterm.c
index a14a8ed..fa7ffe5 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -9513,9 +9513,7 @@ x_free_frame_resources (f)
       XFlush (FRAME_X_DISPLAY (f));
     }

-  if (f->output_data.x->saved_menu_event)
-    xfree (f->output_data.x->saved_menu_event);
-
+  xfree (f->output_data.x->saved_menu_event);
   xfree (f->output_data.x);
   f->output_data.x = NULL;

@@ -10522,10 +10520,8 @@ x_delete_display (dpyinfo)
     xim_close_dpy (dpyinfo);
 #endif

-  if (dpyinfo->x_id_name)
-    xfree (dpyinfo->x_id_name);
-  if (dpyinfo->color_cells)
-    xfree (dpyinfo->color_cells);
+  xfree (dpyinfo->x_id_name);
+  xfree (dpyinfo->color_cells);
   xfree (dpyinfo);
 }

--
1.5.6.rc0.47.g35cb

Re: emacs' turn: remove useless if-before-free tests

[Miles Bader]

Jim Meyering <jim@meyering.net> writes:
> SunOS4 is no longer supported or even used, so that's not an issue.

Er, SunOS 4 is still used here...

[Not that I really disagree with your patch, mind you...]

-Miles

-- 
Do not taunt Happy Fun Ball.

Re: emacs' turn: remove useless if-before-free tests

[Jim Meyering]

Miles Bader <miles@gnu.org> wrote:
> Jim Meyering <jim@meyering.net> writes:
>> SunOS4 is no longer supported or even used, so that's not an issue.
>
> Er, SunOS 4 is still used here...
>
> [Not that I really disagree with your patch, mind you...]

Used how?  So far (in the last year or two), the only reports I've
heard of people using SunOS4 have been from computer museum curators.

Even if someone wants to use emacs with my changes on such a system,
it's not hard: define -Dfree=rpl_free and add this definition:

void
rpl_free (void *p)
{
  if (p)
    free (p);
}

That's what is done by the gnulib "free" module.  It provides the
configure snippet to test for a losing free function, as well as the
replacement, which is compiled only if needed.

But even in coreutils, I've stopped using that module,
as it is now useless, itself, AFAIK.

Re: emacs' turn: remove useless if-before-free tests

[Miles Bader]

Jim Meyering <jim@meyering.net> writes:
>>> SunOS4 is no longer supported or even used, so that's not an issue.
>>
>> Er, SunOS 4 is still used here...
>>
>> [Not that I really disagree with your patch, mind you...]
>
> Used how?

The sunos4 machine I use (at work) is mostly a mail server, though you
can login there too and do stuff if you want.

-Miles

-- 
`Life is a boundless sea of bitterness'

Re: emacs' turn: remove useless if-before-free tests

[Richard M Stallman]

Given that most Emacs code calls xfree rather than free, there are not
many of these if statements, and we rarely add any.
Why remove them?

Re: emacs' turn: remove useless if-before-free tests

[Jim Meyering]

Richard M Stallman <rms@gnu.org> wrote:
> Given that most Emacs code calls xfree rather than free, there are not
> many of these if statements, and we rarely add any.
> Why remove them?

Why *not* remove them?

I find that removing the inherent duplication is safe (mechanical),
and makes the code more readable and more maintainable.  For example,

-  if (x_customization_string)
-    free (x_customization_string);
+  free (x_customization_string);

-  if (fdp->infname != NULL) free (fdp->infname);
-  if (fdp->infabsname != NULL) free (fdp->infabsname);
-  if (fdp->infabsdir != NULL) free (fdp->infabsdir);
-  if (fdp->taggedfname != NULL) free (fdp->taggedfname);
-  if (fdp->prop != NULL) free (fdp->prop);
+  free (fdp->infname);
+  free (fdp->infabsname);
+  free (fdp->infabsdir);
+  free (fdp->taggedfname);
+  free (fdp->prop);

Another advantage is that readers won't wonder why the code is performing
what has long been a useless test -- and then re-read to ensure that
the tested expression and the free argument are properly related.
POSIX requires that free(NULL) be a no-op.

The same arguments hold for xfree, once it has been modified to
accept NULL like free does:

-       if (font->bounds.rows[i])
-         xfree (font->bounds.rows[i]);
+       xfree (font->bounds.rows[i]);

-  if (f->output_data.x->saved_menu_event)
-    xfree (f->output_data.x->saved_menu_event);
-
+  xfree (f->output_data.x->saved_menu_event);

Re: emacs' turn: remove useless if-before-free tests

[Jim Meyering]

Miles Bader <miles@gnu.org> wrote:
> Jim Meyering <jim@meyering.net> writes:
>>>> SunOS4 is no longer supported or even used, so that's not an issue.
>>>
>>> Er, SunOS 4 is still used here...
>>>
>>> [Not that I really disagree with your patch, mind you...]
>>
>> Used how?
>
> The sunos4 machine I use (at work) is mostly a mail server, though you
> can login there too and do stuff if you want.

Amazing.  Sounds risky to use such an old OS for such a task.

Re: emacs' turn: remove useless if-before-free tests

[David Kastrup]

Jim Meyering <jim@meyering.net> writes:

> Miles Bader <miles@gnu.org> wrote:
>> Jim Meyering <jim@meyering.net> writes:
>>>>> SunOS4 is no longer supported or even used, so that's not an issue.
>>>>
>>>> Er, SunOS 4 is still used here...
>>>>
>>>> [Not that I really disagree with your patch, mind you...]
>>>
>>> Used how?
>>
>> The sunos4 machine I use (at work) is mostly a mail server, though you
>> can login there too and do stuff if you want.
>
> Amazing.  Sounds risky to use such an old OS for such a task.

That's what "_you_ can login there too and do stuff if _you_ want"
means.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: emacs' turn: remove useless if-before-free tests

[Miles Bader]

David Kastrup <dak@gnu.org> writes:
>>> The sunos4 machine I use (at work) is mostly a mail server, though you
>>> can login there too and do stuff if you want.
>>
>> Amazing.  Sounds risky to use such an old OS for such a task.
>
> That's what "_you_ can login there too and do stuff if _you_ want"
> means.

So much more convenient than that awful sunos5!

-Miles

-- 
Admiration, n. Our polite recognition of another's resemblance to ourselves.

Re: emacs' turn: remove useless if-before-free tests

[Stefan Monnier]

Sounds like a good cleanup.
Feel free to install it unless there's a strong objection.


        Stefan

Re: emacs' turn: remove useless if-before-free tests

[YAMAMOTO Mitsuharu]

>>>>> On Sun, 01 Jun 2008 17:41:02 -0400, Stefan Monnier <monnier@IRO.UMontreal.CA> said:

> Sounds like a good cleanup.  Feel free to install it unless there's
> a strong objection.

One possible concern is that it will lose programmer's explicit
intention that "this variable may contain either NULL or a return
value of xmalloc" in contrast to "this variable always contain a
return value of xmalloc".

I guess the acceptance of NULL for `free' in POSIX is the reflection
of the fact that `malloc' may return NULL.  If so, we could take a
different policy for `xfree', i.e., abort if NULL, because `xmalloc'
may not return NULL unlike malloc.  This may help us detect the
behavior that is different from programmer's intention.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: emacs' turn: remove useless if-before-free tests

[Miles Bader]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:
> I guess the acceptance of NULL for `free' in POSIX is the reflection
> of the fact that `malloc' may return NULL.  If so, we could take a
> different policy for `xfree', i.e., abort if NULL, because `xmalloc'
> may not return NULL unlike malloc.  This may help us detect the
> behavior that is different from programmer's intention.

xmalloc can return NULL.

I think xmalloc and xfree should act as much like posix malloc/free as
possible, modulo the error-handling behavior of xmalloc.  That will be
the least surprising to programmers using them.  Indeed, we can make
them act like the posix function even on systems whose native malloc is
less so.

-Miles

-- 
Yo mama's so fat when she gets on an elevator it HAS to go down.

Re: emacs' turn: remove useless if-before-free tests

[Stefan Monnier]

>> Sounds like a good cleanup.  Feel free to install it unless there's
>> a strong objection.

> One possible concern is that it will lose programmer's explicit
> intention that "this variable may contain either NULL or a return
> value of xmalloc" in contrast to "this variable always contain a
> return value of xmalloc".

I can't imagine this would be a problem: if the intention matters, the
`if' will cover more than just the call to `free'.


        Stefan

Re: emacs' turn: remove useless if-before-free tests

[YAMAMOTO Mitsuharu]

>>>>> On Mon, 02 Jun 2008 11:05:55 +0900, Miles Bader <miles.bader@necel.com> said:

>> I guess the acceptance of NULL for `free' in POSIX is the
>> reflection of the fact that `malloc' may return NULL.  If so, we
>> could take a different policy for `xfree', i.e., abort if NULL,
>> because `xmalloc' may not return NULL unlike malloc.  This may help
>> us detect the behavior that is different from programmer's
>> intention.

> xmalloc can return NULL.

IIUC, only when the SIZE argument is 0.  I think many programmers have
never cared about the NULL-check of the return value of xmalloc where
they are sure that its argument is positive.  Also we could make
xmalloc to return a non-NULL value for the case SIZE==0.

> I think xmalloc and xfree should act as much like posix malloc/free
> as possible, modulo the error-handling behavior of xmalloc.  That
> will be the least surprising to programmers using them.  Indeed, we
> can make them act like the posix function even on systems whose
> native malloc is less so.

That would be one policy.  What I said is just another possible policy
that could make use of the explicit distinction between `if (p) xfree
(p)' and `xfree (p)' programmers have been made rather than losing
such information just for cleanup.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: emacs' turn: remove useless if-before-free tests

[Jim Meyering]

Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> Sounds like a good cleanup.
> Feel free to install it unless there's a strong objection.

Thanks.  I've committed those three change sets.

Re: emacs' turn: remove useless if-before-free tests

[Richard M Stallman]

    Why *not* remove them?

It is an unnecessary change and a risk of breaking on some platform.

By contrsat, making xfree accept a null argument is unproblematic,
since the function and its callers are 100% under our control.

Re: emacs' turn: remove useless if-before-free tests

[Jim Meyering]

Richard M Stallman <rms@gnu.org> wrote:
>     Why *not* remove them?
>
> It is an unnecessary change and a risk of breaking on some platform.

In my initial message, I included what I thought was enough evidence
to demonstrate that such concerns are anachronistic.

> By contrsat, making xfree accept a null argument is unproblematic,
> since the function and its callers are 100% under our control.

Stefan,
I didn't think there could be a serious objection,
so went ahead and committed the change a few hours ago.

Let me know if you'd like me to revert that part.

If desired, I can propose a patch to do what gnulib's "free" module does,
i.e., detect (at configure time) the losing free function and work around
it via a replacement function that makes "free(NULL)" as a no-op.

That way, we can keep the clean-up without fear that it will cause
trouble even on crufty old systems like SunOS 4.  Though, even emacs
admits that SunOS4 is not supported -- it was removed from emacs' own
list of supported system back in January.  So I'd like to know what
system I'm trying to accommodate if I take the time to write such a patch.

Re: emacs' turn: remove useless if-before-free tests

[Richard M Stallman]

    IIUC, only when the SIZE argument is 0.  I think many programmers have
    never cared about the NULL-check of the return value of xmalloc where
    they are sure that its argument is positive.  Also we could make
    xmalloc to return a non-NULL value for the case SIZE==0.

Maybe we should make xmalloc give an error when the size is 0.

Re: emacs' turn: remove useless if-before-free tests

[Emanuele Giaquinta]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

On Mon, Jun 02, 2008 at 08:14:18AM +0200, Jim Meyering wrote:

> Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> > Sounds like a good cleanup.
> > Feel free to install it unless there's a strong objection.
> 
> Thanks.  I've committed those three change sets.

This patch introduced a problem on os x, free on os x is redirected to
unexmacosx.c:unexec_free, which does not support a NULL argument in an
undumped emacs. The attached patch changes the problematic free call to
xfree, as done a few lines below for another pointer.

Emanuele

[-- Attachment #2: free.diff --]
[-- Type: text/x-diff, Size: 314 bytes --]

diff --git a/src/lread.c b/src/lread.c
index e5e77bc..3e0bd1f 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -1269,7 +1269,7 @@ Return t if the file exists and loads successfully.  */)
 
   UNGCPRO;
 
-  free (saved_doc_string);
+  xfree (saved_doc_string);
   saved_doc_string = 0;
   saved_doc_string_size = 0;
 

Re: emacs' turn: remove useless if-before-free tests

[Jim Meyering]

Emanuele Giaquinta <emanuele.giaquinta@gmail.com> wrote:
> On Mon, Jun 02, 2008 at 08:14:18AM +0200, Jim Meyering wrote:
>
>> Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>> > Sounds like a good cleanup.
>> > Feel free to install it unless there's a strong objection.
>>
>> Thanks.  I've committed those three change sets.
>
> This patch introduced a problem on os x, free on os x is redirected to
> unexmacosx.c:unexec_free, which does not support a NULL argument in an
> undumped emacs. The attached patch changes the problematic free call to
> xfree, as done a few lines below for another pointer.
>
> Emanuele
>
> diff --git a/src/lread.c b/src/lread.c
> index e5e77bc..3e0bd1f 100644
> --- a/src/lread.c
> +++ b/src/lread.c
> @@ -1269,7 +1269,7 @@ Return t if the file exists and loads successfully.  */)
>
>    UNGCPRO;
>
> -  free (saved_doc_string);
> +  xfree (saved_doc_string);
>    saved_doc_string = 0;
>    saved_doc_string_size = 0;

Thanks for catching and fixing that.
If using xfree (with its MALLOC_BLOCK_INPUT guard) is important there,
as it seems to be, then your patch also fixes a platform-independent
race condition bug.

The following change is probably a good idea, too.
It makes unexec_free (NULL) a no-op, just like free (NULL) is:

2008-06-08  Jim Meyering  <meyering@redhat.com>

	make unexec_free handle NULL the same way free does
	* unexmacosx.c (unexec_free): Ignore a NULL argument.

diff --git a/src/unexmacosx.c b/src/unexmacosx.c
index 4662260..57f70f8 100644
--- a/src/unexmacosx.c
+++ b/src/unexmacosx.c
@@ -1318,6 +1318,8 @@ unexec_realloc (void *old_ptr, size_t new_size)
 void
 unexec_free (void *ptr)
 {
+  if (ptr == NULL)
+    return;
   if (in_dumped_exec)
     {
       if (!ptr_in_unexec_regions (ptr))
--
1.5.6.rc1.23.g2f46

Re: emacs' turn: remove useless if-before-free tests

[Jim Meyering]

Jim Meyering <jim@meyering.net> wrote:
> Emanuele Giaquinta <emanuele.giaquinta@gmail.com> wrote:
>> On Mon, Jun 02, 2008 at 08:14:18AM +0200, Jim Meyering wrote:
>>
>>> Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>>> > Sounds like a good cleanup.
>>> > Feel free to install it unless there's a strong objection.
>>>
>>> Thanks.  I've committed those three change sets.
>>
>> This patch introduced a problem on os x, free on os x is redirected to
>> unexmacosx.c:unexec_free, which does not support a NULL argument in an
>> undumped emacs. The attached patch changes the problematic free call to
>> xfree, as done a few lines below for another pointer.
>>
>> Emanuele
>>
>> diff --git a/src/lread.c b/src/lread.c
>> index e5e77bc..3e0bd1f 100644
>> --- a/src/lread.c
>> +++ b/src/lread.c
>> @@ -1269,7 +1269,7 @@ Return t if the file exists and loads successfully.  */)
>>
>>    UNGCPRO;
>>
>> -  free (saved_doc_string);
>> +  xfree (saved_doc_string);
>>    saved_doc_string = 0;
>>    saved_doc_string_size = 0;
>
> Thanks for catching and fixing that.
> If using xfree (with its MALLOC_BLOCK_INPUT guard) is important there,
> as it seems to be, then your patch also fixes a platform-independent
> race condition bug.
>
> The following change is probably a good idea, too.
> It makes unexec_free (NULL) a no-op, just like free (NULL) is:
>
> 2008-06-08  Jim Meyering  <meyering@redhat.com>
>
> 	make unexec_free handle NULL the same way free does
> 	* unexmacosx.c (unexec_free): Ignore a NULL argument.

If no one objects soon, I'll commit both our changes.

> diff --git a/src/unexmacosx.c b/src/unexmacosx.c
> index 4662260..57f70f8 100644
> --- a/src/unexmacosx.c
> +++ b/src/unexmacosx.c
> @@ -1318,6 +1318,8 @@ unexec_realloc (void *old_ptr, size_t new_size)
>  void
>  unexec_free (void *ptr)
>  {
> +  if (ptr == NULL)
> +    return;
>    if (in_dumped_exec)
>      {
>        if (!ptr_in_unexec_regions (ptr))
> --
> 1.5.6.rc1.23.g2f46

Re: emacs' turn: remove useless if-before-free tests

[Jim Meyering]

Jim Meyering <jim@meyering.net> wrote:
> Jim Meyering <jim@meyering.net> wrote:
>> Emanuele Giaquinta <emanuele.giaquinta@gmail.com> wrote:
>>> On Mon, Jun 02, 2008 at 08:14:18AM +0200, Jim Meyering wrote:
>>>
>>>> Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>>>> > Sounds like a good cleanup.
>>>> > Feel free to install it unless there's a strong objection.
>>>>
>>>> Thanks.  I've committed those three change sets.
>>>
>>> This patch introduced a problem on os x, free on os x is redirected to
>>> unexmacosx.c:unexec_free, which does not support a NULL argument in an
>>> undumped emacs. The attached patch changes the problematic free call to
>>> xfree, as done a few lines below for another pointer.
>>>
>>> Emanuele
>>>
>>> diff --git a/src/lread.c b/src/lread.c
>>> index e5e77bc..3e0bd1f 100644
>>> --- a/src/lread.c
>>> +++ b/src/lread.c
>>> @@ -1269,7 +1269,7 @@ Return t if the file exists and loads successfully.  */)
>>>
>>>    UNGCPRO;
>>>
>>> -  free (saved_doc_string);
>>> +  xfree (saved_doc_string);
>>>    saved_doc_string = 0;
>>>    saved_doc_string_size = 0;
>>
>> Thanks for catching and fixing that.
>> If using xfree (with its MALLOC_BLOCK_INPUT guard) is important there,
>> as it seems to be, then your patch also fixes a platform-independent
>> race condition bug.
>>
>> The following change is probably a good idea, too.
>> It makes unexec_free (NULL) a no-op, just like free (NULL) is:
>>
>> 2008-06-08  Jim Meyering  <meyering@redhat.com>
>>
>> 	make unexec_free handle NULL the same way free does
>> 	* unexmacosx.c (unexec_free): Ignore a NULL argument.
>
> If no one objects soon, I'll commit both our changes.

Done.