[RFA] Eliminate warnings about snprintf declaration

[RFA] Eliminate warnings about snprintf declaration

[Eli Zaretskii]

> Date: Mon, 15 Nov 2004 19:36:45 -0500
> From: Andrew Cagney <cagney@gnu.org>
> Cc: gdb@sources.redhat.com
> 
> Eli Zaretskii wrote:
> > When I build GDB with DJGPP, GCC whines like so:
> > 
> >     remote.c: In function `remote_xfer_partial':
> >     remote.c:4920: warning: implicit declaration of function `snprintf'
> > 
> > (I see this warning for other source files as well.)
> > 
> > This happens because DJGPP doesn't have `snprintf' in its library, so
> > the libiberty version is used.  But there's no prototype for the
> > libiberty `snprintf' anywhere, thus the warning.  The code seems to be
> > relying on getting the prototype from stdio.h, but if the library
> > doesn't have the function, we cannot expect to have the prototype on a
> > standard header.
> > 
> > What would be the canonical way to fix that?
> 
> include/libiberty.h, for instance:
> 
> > #if !HAVE_DECL_ASPRINTF
> > /* Like sprintf but provides a pointer to malloc'd storage, which must
> >    be freed by the caller.  */
> > 
> > extern int asprintf PARAMS ((char **, const char *, ...)) ATTRIBUTE_PRINTF_2;
> > #endif

Okay, here're the patches to add snprintf and vsnprintf to the list of
functions whose declarations are checked at configure time.  Comments?


libiberty/ChangeLog:
2005-05-12  Eli Zaretskii  <eliz@gnu.org>

	* configure.ac: Add snprintf and vsnprintf to AC_CHEK_DECLS.


include/ChangeLog:
2005-05-12  Eli Zaretskii  <eliz@gnu.org>

	* libiberty.h (snprintf) [!HAVE_DECL_SNPRINTF]: Add a prototype.
	(vsnprintf) [!HAVE_DECL_VSNPRINTF]: Add a prototype.


Index: libiberty/configure.ac
===================================================================
RCS file: /cvs/src/src/libiberty/configure.ac,v
retrieving revision 1.22
diff -u -r1.22 configure.ac
--- libiberty/configure.ac	7 May 2005 02:00:41 -0000	1.22
+++ libiberty/configure.ac	12 May 2005 14:55:28 -0000
@@ -282,7 +282,7 @@
   sysconf times sbrk gettimeofday ffs snprintf vsnprintf \
   pstat_getstatic pstat_getdynamic sysmp getsysinfo table sysctl wait3 wait4 \
   realpath canonicalize_file_name __fsetlocking)
-  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf])
+  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf, snprintf, vsnprintf])
   AC_DEFINE(HAVE_SYS_ERRLIST, 1, [Define if you have the sys_errlist variable.])
   AC_DEFINE(HAVE_SYS_NERR,    1, [Define if you have the sys_nerr variable.])
   AC_DEFINE(HAVE_SYS_SIGLIST, 1, [Define if you have the sys_siglist variable.])
@@ -518,7 +518,7 @@
     [AC_MSG_RESULT([no])])
 
   AC_CHECK_FUNCS($checkfuncs)
-  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf])
+  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf, snprintf, vsnprintf])
   libiberty_NEED_DECLARATION(canonicalize_file_name)
 fi
 
Index: include/libiberty.h
===================================================================
RCS file: /cvs/src/src/include/libiberty.h,v
retrieving revision 1.43
diff -u -r1.43 libiberty.h
--- include/libiberty.h	10 May 2005 10:21:08 -0000	1.43
+++ include/libiberty.h	12 May 2005 14:56:04 -0000
@@ -528,6 +528,24 @@
   ATTRIBUTE_PRINTF(2,0);
 #endif
 
+#if !HAVE_DECL_SNPRINTF
+# ifdef HAVE_DECL_SNPRINTF
+  extern int snprintf ();
+# else
+/* Like sprintf but prints at most N characters.  */
+  extern int snprintf (char *, size_t, const char *, ...) ATTRIBUTE_PRINTF_3;
+# endif
+#endif
+
+#if !HAVE_DECL_VSNPRINTF
+# ifdef HAVE_DECL_VSNPRINTF
+  extern int vsnprintf ();
+# else
+/* Like vsprintf but prints at most N characters.  */
+  extern int vsnprintf (char *, size_t, const char *, va_list);
+# endif
+#endif
+
 #define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
 
 /* Drastically simplified alloca configurator.  If we're using GCC,

Re: [RFA] Eliminate warnings about snprintf declaration

[Daniel Jacobowitz]

On Thu, May 12, 2005 at 05:55:49PM +0300, Eli Zaretskii wrote:
> Okay, here're the patches to add snprintf and vsnprintf to the list of
> functions whose declarations are checked at configure time.  Comments?

I recommend copying the libiberty maintainers directly to get their
attention.

Also, this isn't useful:

> @@ -282,7 +282,7 @@
>    sysconf times sbrk gettimeofday ffs snprintf vsnprintf \
>    pstat_getstatic pstat_getdynamic sysmp getsysinfo table sysctl wait3 wait4 \
>    realpath canonicalize_file_name __fsetlocking)
> -  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf])
> +  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf, snprintf, vsnprintf])
>    AC_DEFINE(HAVE_SYS_ERRLIST, 1, [Define if you have the sys_errlist variable.])
>    AC_DEFINE(HAVE_SYS_NERR,    1, [Define if you have the sys_nerr variable.])
>    AC_DEFINE(HAVE_SYS_SIGLIST, 1, [Define if you have the sys_siglist variable.])
> @@ -518,7 +518,7 @@
>      [AC_MSG_RESULT([no])])
>  
>    AC_CHECK_FUNCS($checkfuncs)
> -  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf])
> +  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf, snprintf, vsnprintf])
>    libiberty_NEED_DECLARATION(canonicalize_file_name)
>  fi

That will only affect libiberty.h when building libiberty; it won't
affect the users of libiberty.

> +#if !HAVE_DECL_SNPRINTF
> +# ifdef HAVE_DECL_SNPRINTF
> +  extern int snprintf ();
> +# else
> +/* Like sprintf but prints at most N characters.  */
> +  extern int snprintf (char *, size_t, const char *, ...) ATTRIBUTE_PRINTF_3;
> +# endif
> +#endif
> +
> +#if !HAVE_DECL_VSNPRINTF
> +# ifdef HAVE_DECL_VSNPRINTF
> +  extern int vsnprintf ();
> +# else
> +/* Like vsprintf but prints at most N characters.  */
> +  extern int vsnprintf (char *, size_t, const char *, va_list);
> +# endif
> +#endif
> +

I believe you've got this logic reversed.  You also don't want or need
the non-prototype - it's only needed for basename because basename
returns a pointer.  How about this, based on the later examples in the
file:

#if defined (HAVE_DECL_SPRINTF) && !HAVE_DECL_SNPRINTF
/* Like sprintf but prints at most N characters.  */
extern int snprintf (char *, size_t, const char *, ...) ATTRIBUTE_PRINTF_3;
#endif

#if defined (HAVE_DECL_VSNPRINTF) && !HAVE_DECL_VSNPRINTF
/* Like vsprintf but prints at most N characters.  */
extern int vsnprintf (char *, size_t, const char *, va_list);
#endif

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] Eliminate warnings about snprintf declaration

[Eli Zaretskii]

> Date: Thu, 12 May 2005 11:08:04 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gcc-patches@gcc.gnu.org, gdb-patches@sources.redhat.com
> 
> I recommend copying the libiberty maintainers directly to get their
> attention.

Errr... who are they?  libiberty/README says to send patches to
gcc-patches, which I did.  What am I missing?

> Also, this isn't useful:
> 
> > @@ -282,7 +282,7 @@
> >    sysconf times sbrk gettimeofday ffs snprintf vsnprintf \
> >    pstat_getstatic pstat_getdynamic sysmp getsysinfo table sysctl wait3 wait4 \
> >    realpath canonicalize_file_name __fsetlocking)
> > -  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf])
> > +  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf, snprintf, vsnprintf])
> >    AC_DEFINE(HAVE_SYS_ERRLIST, 1, [Define if you have the sys_errlist variable.])
> >    AC_DEFINE(HAVE_SYS_NERR,    1, [Define if you have the sys_nerr variable.])
> >    AC_DEFINE(HAVE_SYS_SIGLIST, 1, [Define if you have the sys_siglist variable.])
> > @@ -518,7 +518,7 @@
> >      [AC_MSG_RESULT([no])])
> >  
> >    AC_CHECK_FUNCS($checkfuncs)
> > -  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf])
> > +  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf, snprintf, vsnprintf])
> >    libiberty_NEED_DECLARATION(canonicalize_file_name)
> >  fi
> 
> That will only affect libiberty.h when building libiberty; it won't
> affect the users of libiberty.

Then how does one cause libiberty/configure to check for these
declarations and edit libiberty/config.h accordingly?  I thought one
should modify configure.ac and the regenerate configure, no?

> I believe you've got this logic reversed.  You also don't want or need
> the non-prototype - it's only needed for basename because basename
> returns a pointer.  How about this, based on the later examples in the
> file:

Okay.  But I guess I should wait for approval by libiberty maintainers
first.

Thanks.

Re: [RFA] Eliminate warnings about snprintf declaration

[Daniel Jacobowitz]

On Thu, May 12, 2005 at 06:38:38PM +0300, Eli Zaretskii wrote:
> > Date: Thu, 12 May 2005 11:08:04 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: gcc-patches@gcc.gnu.org, gdb-patches@sources.redhat.com
> > 
> > I recommend copying the libiberty maintainers directly to get their
> > attention.
> 
> Errr... who are they?  libiberty/README says to send patches to
> gcc-patches, which I did.  What am I missing?

DJ and Ian.  I missed the CC to gcc-patches, sorry.  That's plenty.

> > That will only affect libiberty.h when building libiberty; it won't
> > affect the users of libiberty.
> 
> Then how does one cause libiberty/configure to check for these
> declarations and edit libiberty/config.h accordingly?  I thought one
> should modify configure.ac and the regenerate configure, no?

It doesn't matter.  libiberty/config.h is not a public file; it is only
used while building libiberty.  Any project that wants a prototype for
these functions is responsible for the check in its own private
config.h.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] Eliminate warnings about snprintf declaration

[Eli Zaretskii]

> Date: Thu, 12 May 2005 11:47:16 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gcc-patches@gcc.gnu.org, gdb-patches@sources.redhat.com
> 
> > > That will only affect libiberty.h when building libiberty; it won't
> > > affect the users of libiberty.
> > 
> > Then how does one cause libiberty/configure to check for these
> > declarations and edit libiberty/config.h accordingly?  I thought one
> > should modify configure.ac and the regenerate configure, no?
> 
> It doesn't matter.  libiberty/config.h is not a public file; it is only
> used while building libiberty.  Any project that wants a prototype for
> these functions is responsible for the check in its own private
> config.h.

I guess I'm missing something obvious, since I still don't get it.

Here's the process of getting these declarations as I understand it:

  . the configure script checks whether asprintf, basename, ffs,
    etc. are declared on standard headers

  . it then edits config.in into config.h and adds the appropriate
    definitions for HAVE_DECL_ASPRINTF, HAVE_DECL_BASENAME, etc.

  . include/libiberty.h then enables its own declarations for those
    platforms for which HAVE_DECL_ASPRINTF etc. say there's a need for
    a declaration

Is the above correct?  If so, I need to cause two changes, in addition
to modifying include/libiberty.h:

 1) cause the configure script check for declarations of 2 additional
    functions--snprintf and vsnprintf

 2) add #undef's for HAVE_DECL_SNPRINTF and HAVE_DECL_VSNPRINTF which
    will be edited by the configure script

How do I make these two changes, if not by modifications to
libiberty/configure.ac followed by running Autoconf?  Are you saying
that I should change gdb/configure.ac (or some other configure)
instead?

Sorry for being so slow.

Re: [RFA] Eliminate warnings about snprintf declaration

[Daniel Jacobowitz]

On Thu, May 12, 2005 at 07:16:34PM +0300, Eli Zaretskii wrote:
> I guess I'm missing something obvious, since I still don't get it.
> 
> Here's the process of getting these declarations as I understand it:
> 
>   . the configure script checks whether asprintf, basename, ffs,
>     etc. are declared on standard headers
> 
>   . it then edits config.in into config.h and adds the appropriate
>     definitions for HAVE_DECL_ASPRINTF, HAVE_DECL_BASENAME, etc.
> 
>   . include/libiberty.h then enables its own declarations for those
>     platforms for which HAVE_DECL_ASPRINTF etc. say there's a need for
>     a declaration
> 
> Is the above correct?  If so, I need to cause two changes, in addition

The above is correct.

> to modifying include/libiberty.h:
> 
>  1) cause the configure script check for declarations of 2 additional
>     functions--snprintf and vsnprintf
> 
>  2) add #undef's for HAVE_DECL_SNPRINTF and HAVE_DECL_VSNPRINTF which
>     will be edited by the configure script
> 
> How do I make these two changes, if not by modifications to
> libiberty/configure.ac followed by running Autoconf?  Are you saying
> that I should change gdb/configure.ac (or some other configure)
> instead?

That is also correct.

The principle is that any project which uses libiberty, which wants
these definitions, must request them.  Libiberty's config.h is included
by libiberty source files, before they include libiberty.h.  GDB's
config.h is included by GDB source fles before they include
libiberty.h.  So, if GDB needs the prototype, it is GDB's
responsibility to request it.

Basename is an unfortunate example; you can find some discussion of why
on binutils@ over the last few weeks.  That's because it returns a
pointer, so a prototype _must_ be available in order to safely use it.
This problem doesn't apply to *printf.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] Eliminate warnings about snprintf declaration

[Eli Zaretskii]

> Date: Thu, 12 May 2005 12:24:53 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gcc-patches@gcc.gnu.org, gdb-patches@sources.redhat.com
> 
> The principle is that any project which uses libiberty, which wants
> these definitions, must request them.  Libiberty's config.h is included
> by libiberty source files, before they include libiberty.h.  GDB's
> config.h is included by GDB source fles before they include
> libiberty.h.  So, if GDB needs the prototype, it is GDB's
> responsibility to request it.

So I need to add these functions to gdb/configure.ac _and_ to
libiberty/configure.ac; got it.  I will post a revised patch soon.

Thanks for your patience.

Re: [RFA] Eliminate warnings about snprintf declaration

[Daniel Jacobowitz]

On Thu, May 12, 2005 at 11:40:00PM +0300, Eli Zaretskii wrote:
> > Date: Thu, 12 May 2005 12:24:53 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: gcc-patches@gcc.gnu.org, gdb-patches@sources.redhat.com
> > 
> > The principle is that any project which uses libiberty, which wants
> > these definitions, must request them.  Libiberty's config.h is included
> > by libiberty source files, before they include libiberty.h.  GDB's
> > config.h is included by GDB source fles before they include
> > libiberty.h.  So, if GDB needs the prototype, it is GDB's
> > responsibility to request it.
> 
> So I need to add these functions to gdb/configure.ac _and_ to
> libiberty/configure.ac; got it.  I will post a revised patch soon.
> 
> Thanks for your patience.

Almost - you don't actually need them in libiberty/configure.ac, unless
you see warnings about them during the build of libiberty.  To make GDB
-Werror happy, all you need is gdb/configure.ac and
include/libiberty.h.

Thank you for taking care of it!

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] Eliminate warnings about snprintf declaration

[DJ Delorie]

> Almost - you don't actually need them in libiberty/configure.ac,
> unless you see warnings about them during the build of libiberty.
> To make GDB -Werror happy, all you need is gdb/configure.ac and
> include/libiberty.h.

Libiberty doesn't currently build with -Werror, but it's probably a
good idea to ac_test for everything in libiberty.h anyway.  It serves
as both a sanity check and an example to others.

Re: [RFA] Eliminate warnings about snprintf declaration

[Eli Zaretskii]

> Date: Thu, 12 May 2005 16:51:03 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gcc-patches@gcc.gnu.org, gdb-patches@sources.redhat.com
> 
> > So I need to add these functions to gdb/configure.ac _and_ to
> > libiberty/configure.ac; got it.  I will post a revised patch soon.
> > 
> > Thanks for your patience.
> 
> Almost - you don't actually need them in libiberty/configure.ac, unless
> you see warnings about them during the build of libiberty.  To make GDB
> -Werror happy, all you need is gdb/configure.ac and
> include/libiberty.h.

Actually, I found that a simpler patch to include/libiberty.h (below)
is all that is needed to fix all the warnings about snprintf and
vsnprintf.  libiberty.h already does that for asprintf and vasprintf,
so I think we can use the same method for snprintf and vsnprintf; no
need to change any configure.ac files.

Okay?

2005-05-14  Eli Zaretskii  <eliz@gnu.org>

	* libiberty.h: (snprintf) [!HAVE_DECL_SNPRINTF]: Declare if
	needed.
	(vsnprintf) [!HAVE_DECL_VSNPRINTF]: Declare if needed.

Index: include/libiberty.h
===================================================================
RCS file: /cvs/src/src/include/libiberty.h,v
retrieving revision 1.44
diff -u -r1.44 libiberty.h
--- include/libiberty.h	12 May 2005 20:00:35 -0000	1.44
+++ include/libiberty.h	14 May 2005 10:08:37 -0000
@@ -531,6 +531,16 @@
   ATTRIBUTE_PRINTF(2,0);
 #endif
 
+#if !HAVE_DECL_SNPRINTF
+/* Like sprintf but prints at most N characters.  */
+extern int snprintf (char *, size_t, const char *, ...) ATTRIBUTE_PRINTF_3;
+#endif
+
+#if !HAVE_DECL_VSNPRINTF
+/* Like vsprintf but prints at most N characters.  */
+extern int vsnprintf (char *, size_t, const char *, va_list);
+#endif
+
 #define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
 
 /* Drastically simplified alloca configurator.  If we're using GCC,

Re: [RFA] Eliminate warnings about snprintf declaration

[Daniel Jacobowitz]

On Sat, May 14, 2005 at 01:05:51PM +0300, Eli Zaretskii wrote:
> > Date: Thu, 12 May 2005 16:51:03 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: gcc-patches@gcc.gnu.org, gdb-patches@sources.redhat.com
> > 
> > > So I need to add these functions to gdb/configure.ac _and_ to
> > > libiberty/configure.ac; got it.  I will post a revised patch soon.
> > > 
> > > Thanks for your patience.
> > 
> > Almost - you don't actually need them in libiberty/configure.ac, unless
> > you see warnings about them during the build of libiberty.  To make GDB
> > -Werror happy, all you need is gdb/configure.ac and
> > include/libiberty.h.
> 
> Actually, I found that a simpler patch to include/libiberty.h (below)
> is all that is needed to fix all the warnings about snprintf and
> vsnprintf.  libiberty.h already does that for asprintf and vasprintf,
> so I think we can use the same method for snprintf and vsnprintf; no
> need to change any configure.ac files.
> 
> Okay?
> 
> 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> 
> 	* libiberty.h: (snprintf) [!HAVE_DECL_SNPRINTF]: Declare if
> 	needed.
> 	(vsnprintf) [!HAVE_DECL_VSNPRINTF]: Declare if needed.

Note that this will only work if there are no systems which declare
snprintf with an incorrect prototype.  For instance, a missing const on
the format string in a system header would now generate an error.  For
asprintf this isn't an issue, because it's not a standard function;
it's only provided by glibc.

I don't know whether that's a real problem though.  I'll let DJ answer
that :-)

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] Eliminate warnings about snprintf declaration

[Eli Zaretskii]

> Date: Sat, 14 May 2005 10:12:04 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gcc-patches@gcc.gnu.org, gdb-patches@sources.redhat.com
> 
> > Actually, I found that a simpler patch to include/libiberty.h (below)
> > is all that is needed to fix all the warnings about snprintf and
> > vsnprintf.  libiberty.h already does that for asprintf and vasprintf,
> > so I think we can use the same method for snprintf and vsnprintf; no
> > need to change any configure.ac files.
> 
> > 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> > 
> > 	* libiberty.h: (snprintf) [!HAVE_DECL_SNPRINTF]: Declare if
> > 	needed.
> > 	(vsnprintf) [!HAVE_DECL_VSNPRINTF]: Declare if needed.
> 
> Note that this will only work if there are no systems which declare
> snprintf with an incorrect prototype.  For instance, a missing const on
> the format string in a system header would now generate an error.  For
> asprintf this isn't an issue, because it's not a standard function;
> it's only provided by glibc.

Actually, what I wrote was completely bogus: without adding these
functions to AC_CHECK_DECLS, the respective HAVE_DECL_* macros will
never get defined, so what I suggested is equivalent to including
these prototypes unconditionally, which is certainly not what I meant.
Sorry.

Here's a revised patch, now together with the configury stuff.


include/ChangeLog:
2005-05-14  Eli Zaretskii  <eliz@gnu.org>

	* libiberty.h: (snprintf) [!HAVE_DECL_SNPRINTF]: Declare if
	needed.
	(vsnprintf) [!HAVE_DECL_VSNPRINTF]: Declare if needed.


bfd/ChangeLog:
2005-05-14  Eli Zaretskii  <eliz@gnu.org>

	* configure.in: Add snprintf and vsnprintf to AC_CHECK_DECLS.


gdb/ChangeLog:
2005-05-14  Eli Zaretskii  <eliz@gnu.org>

	* configure.ac: Add snprintf and vsnprintf to AC_CHECK_DECLS.


Index: include/libiberty.h
===================================================================
RCS file: /cvs/src/src/include/libiberty.h,v
retrieving revision 1.44
diff -u -r1.44 libiberty.h
--- include/libiberty.h	12 May 2005 20:00:35 -0000	1.44
+++ include/libiberty.h	14 May 2005 16:09:49 -0000
@@ -531,6 +531,16 @@
   ATTRIBUTE_PRINTF(2,0);
 #endif
 
+#if defined(HAVE_DECL_SNPRINTF) && !HAVE_DECL_SNPRINTF
+/* Like sprintf but prints at most N characters.  */
+extern int snprintf (char *, size_t, const char *, ...) ATTRIBUTE_PRINTF_3;
+#endif
+
+#if defined(HAVE_DECL_VSNPRINTF) && !HAVE_DECL_VSNPRINTF
+/* Like vsprintf but prints at most N characters.  */
+extern int vsnprintf (char *, size_t, const char *, va_list);
+#endif
+
 #define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
 
 /* Drastically simplified alloca configurator.  If we're using GCC,
Index: bfd/configure.in
===================================================================
RCS file: /cvs/src/src/bfd/configure.in,v
retrieving revision 1.183
diff -u -r1.183 configure.in
--- bfd/configure.in	10 May 2005 01:27:53 -0000	1.183
+++ bfd/configure.in	14 May 2005 16:10:11 -0000
@@ -144,6 +144,8 @@
 AC_CHECK_DECLS(realloc)
 AC_CHECK_DECLS(stpcpy)
 AC_CHECK_DECLS(strstr)
+AC_CHECK_DECLS(snprintf)
+AC_CHECK_DECLS(vsnprintf)
 
 # If we are configured native, pick a core file support file.
 COREFILE=
Index: gdb/configure.ac
===================================================================
RCS file: /cvs/src/src/gdb/configure.ac,v
retrieving revision 1.18
diff -u -r1.18 configure.ac
--- gdb/configure.ac	21 Apr 2005 05:34:33 -0000	1.18
+++ gdb/configure.ac	14 May 2005 16:10:31 -0000
@@ -418,7 +418,7 @@
 
 AC_CHECK_DECLS([free, malloc, realloc])
 AC_CHECK_DECLS([strerror, strstr])
-AC_CHECK_DECLS(getopt)
+AC_CHECK_DECLS([getopt, snprintf, vsnprintf])
 
 # ----------------------- #
 # Checks for structures.  #
Index: libiberty/configure.ac
===================================================================
RCS file: /cvs/src/src/libiberty/configure.ac,v
retrieving revision 1.22
diff -u -r1.22 configure.ac
--- libiberty/configure.ac	7 May 2005 02:00:41 -0000	1.22
+++ libiberty/configure.ac	14 May 2005 16:10:46 -0000
@@ -282,7 +282,7 @@
   sysconf times sbrk gettimeofday ffs snprintf vsnprintf \
   pstat_getstatic pstat_getdynamic sysmp getsysinfo table sysctl wait3 wait4 \
   realpath canonicalize_file_name __fsetlocking)
-  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf])
+  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf, snprintf, vsnprintf])
   AC_DEFINE(HAVE_SYS_ERRLIST, 1, [Define if you have the sys_errlist variable.])
   AC_DEFINE(HAVE_SYS_NERR,    1, [Define if you have the sys_nerr variable.])
   AC_DEFINE(HAVE_SYS_SIGLIST, 1, [Define if you have the sys_siglist variable.])
@@ -518,7 +518,7 @@
     [AC_MSG_RESULT([no])])
 
   AC_CHECK_FUNCS($checkfuncs)
-  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf])
+  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf, snprintf, vsnprintf])
   libiberty_NEED_DECLARATION(canonicalize_file_name)
 fi
 

Re: [RFA] Eliminate warnings about snprintf declaration

[Ian Lance Taylor]

"Eli Zaretskii" <eliz@gnu.org> writes:

> include/ChangeLog:
> 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> 
> 	* libiberty.h: (snprintf) [!HAVE_DECL_SNPRINTF]: Declare if
> 	needed.
> 	(vsnprintf) [!HAVE_DECL_VSNPRINTF]: Declare if needed.
> 
> 
> bfd/ChangeLog:
> 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> 
> 	* configure.in: Add snprintf and vsnprintf to AC_CHECK_DECLS.

These patches are approved, plus the change to libiberty/configure.ac
which needs the obvious ChangeLog entry.  Thanks.

> gdb/ChangeLog:
> 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> 
> 	* configure.ac: Add snprintf and vsnprintf to AC_CHECK_DECLS.

I can't approve this one, but it looks fine to me.

Ian

Re: [RFA] Eliminate warnings about snprintf declaration

[Daniel Jacobowitz]

On Sun, May 15, 2005 at 12:29:11PM -0400, Ian Lance Taylor wrote:
> "Eli Zaretskii" <eliz@gnu.org> writes:
> 
> > include/ChangeLog:
> > 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> > 
> > 	* libiberty.h: (snprintf) [!HAVE_DECL_SNPRINTF]: Declare if
> > 	needed.
> > 	(vsnprintf) [!HAVE_DECL_VSNPRINTF]: Declare if needed.
> > 
> > 
> > bfd/ChangeLog:
> > 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> > 
> > 	* configure.in: Add snprintf and vsnprintf to AC_CHECK_DECLS.
> 
> These patches are approved, plus the change to libiberty/configure.ac
> which needs the obvious ChangeLog entry.  Thanks.

Eli, please mention regenerating configure in the ChangeLog.

> > gdb/ChangeLog:
> > 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> > 
> > 	* configure.ac: Add snprintf and vsnprintf to AC_CHECK_DECLS.
> 
> I can't approve this one, but it looks fine to me.

Eli can approve it himself; or I will :-)  Same comment as above
applies; please mention configure.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] Eliminate warnings about snprintf declaration

[Eli Zaretskii]

> Date: Sun, 15 May 2005 13:04:35 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, gcc-patches@gcc.gnu.org,
> 	gdb-patches@sourceware.org, binutils@sourceware.org
> 
> > > bfd/ChangeLog:
> > > 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> > > 
> > > 	* configure.in: Add snprintf and vsnprintf to AC_CHECK_DECLS.
> > 
> > These patches are approved, plus the change to libiberty/configure.ac
> > which needs the obvious ChangeLog entry.  Thanks.
> 
> Eli, please mention regenerating configure in the ChangeLog.

I didn't send patches for the (regenerated) configure and config.in.
Should I do that, given that I cannot commit them anyway?

> > > gdb/ChangeLog:
> > > 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> > > 
> > > 	* configure.ac: Add snprintf and vsnprintf to AC_CHECK_DECLS.
> > 
> > I can't approve this one, but it looks fine to me.
> 
> Eli can approve it himself; or I will :-)

True; but given the problems in my original patch, I prefered to get
this by you-all.

> Same comment as above applies; please mention configure.

(And config.in, right?)  Will do.

Re: [RFA] Eliminate warnings about snprintf declaration

[Daniel Jacobowitz]

On Sun, May 15, 2005 at 10:40:59PM +0300, Eli Zaretskii wrote:
> > Date: Sun, 15 May 2005 13:04:35 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: Eli Zaretskii <eliz@gnu.org>, gcc-patches@gcc.gnu.org,
> > 	gdb-patches@sourceware.org, binutils@sourceware.org
> > 
> > > > bfd/ChangeLog:
> > > > 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> > > > 
> > > > 	* configure.in: Add snprintf and vsnprintf to AC_CHECK_DECLS.
> > > 
> > > These patches are approved, plus the change to libiberty/configure.ac
> > > which needs the obvious ChangeLog entry.  Thanks.
> > 
> > Eli, please mention regenerating configure in the ChangeLog.
> 
> I didn't send patches for the (regenerated) configure and config.in.
> Should I do that, given that I cannot commit them anyway?

No, you don't need to.  If you mention them in the ChangeLog, that'll
serve as a hint to whoever commits the patch which files need to be
regenerated.

> > Same comment as above applies; please mention configure.
> 
> (And config.in, right?)  Will do.

Good catch.  Yes, thank you.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] Eliminate warnings about snprintf declaration

[Eli Zaretskii]

> Date: Sun, 15 May 2005 15:55:23 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Ian Lance Taylor <ian@airs.com>, gcc-patches@gcc.gnu.org,
> 	gdb-patches@sourceware.org, binutils@sourceware.org
> 
> On Sun, May 15, 2005 at 10:40:59PM +0300, Eli Zaretskii wrote:
> > > Date: Sun, 15 May 2005 13:04:35 -0400
> > > From: Daniel Jacobowitz <drow@false.org>
> > > Cc: Eli Zaretskii <eliz@gnu.org>, gcc-patches@gcc.gnu.org,
> > > 	gdb-patches@sourceware.org, binutils@sourceware.org
> > > 
> > > > > bfd/ChangeLog:
> > > > > 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> > > > > 
> > > > > 	* configure.in: Add snprintf and vsnprintf to AC_CHECK_DECLS.
> > > > 
> > > > These patches are approved, plus the change to libiberty/configure.ac
> > > > which needs the obvious ChangeLog entry.  Thanks.
> > > 
> > > Eli, please mention regenerating configure in the ChangeLog.
> > 
> > I didn't send patches for the (regenerated) configure and config.in.
> > Should I do that, given that I cannot commit them anyway?
> 
> No, you don't need to.  If you mention them in the ChangeLog, that'll
> serve as a hint to whoever commits the patch which files need to be
> regenerated.
> 
> > > Same comment as above applies; please mention configure.
> > 
> > (And config.in, right?)  Will do.
> 
> Good catch.  Yes, thank you.

Okay, here's the revised patch with revised ChangeLog entries.  I will
commit the GDB part shortly, but I need someone else to commit the
patches in bfd/, include/, and libiberty/.  TIA

libiberty/ChangeLog:
2005-05-20  Eli Zaretskii  <eliz@gnu.org>

	* configure.ac: Add snprintf and vsnprintf to AC_CHEK_DECLS.
	* config.in, configure: Regenerate.

include/ChangeLog:
2005-05-20  Eli Zaretskii  <eliz@gnu.org>

	* libiberty.h: (snprintf) [!HAVE_DECL_SNPRINTF]: Declare if
	needed.
	(vsnprintf) [!HAVE_DECL_VSNPRINTF]: Declare if needed.

gdb/ChangeLog:
2005-05-20  Eli Zaretskii  <eliz@gnu.org>

	* configure.ac: Add snprintf and vsnprintf to AC_CHECK_DECLS.
	* config.in, configure: Regenerate.

bfd/ChangeLog:
2005-05-20  Eli Zaretskii  <eliz@gnu.org>

	* configure.in: Add snprintf and vsnprintf to AC_CHECK_DECLS.
	* config.in, configure: Regenerate.


Index: bfd/configure.in
===================================================================
RCS file: /cvs/src/src/bfd/configure.in,v
retrieving revision 1.183
diff -u -r1.183 configure.in
--- bfd/configure.in	10 May 2005 01:27:53 -0000	1.183
+++ bfd/configure.in	20 May 2005 11:16:55 -0000
@@ -144,6 +144,8 @@
 AC_CHECK_DECLS(realloc)
 AC_CHECK_DECLS(stpcpy)
 AC_CHECK_DECLS(strstr)
+AC_CHECK_DECLS(snprintf)
+AC_CHECK_DECLS(vsnprintf)
 
 # If we are configured native, pick a core file support file.
 COREFILE=
cvs diff: I know nothing about gdb/configure.in
Index: include/libiberty.h
===================================================================
RCS file: /cvs/src/src/include/libiberty.h,v
retrieving revision 1.44
diff -u -r1.44 libiberty.h
--- include/libiberty.h	12 May 2005 20:00:35 -0000	1.44
+++ include/libiberty.h	20 May 2005 11:20:18 -0000
@@ -531,6 +531,16 @@
   ATTRIBUTE_PRINTF(2,0);
 #endif
 
+#if defined(HAVE_DECL_SNPRINTF) && !HAVE_DECL_SNPRINTF
+/* Like sprintf but prints at most N characters.  */
+extern int snprintf (char *, size_t, const char *, ...) ATTRIBUTE_PRINTF_3;
+#endif
+
+#if defined(HAVE_DECL_VSNPRINTF) && !HAVE_DECL_VSNPRINTF
+/* Like vsprintf but prints at most N characters.  */
+extern int vsnprintf (char *, size_t, const char *, va_list);
+#endif
+
 #define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
 
 /* Drastically simplified alloca configurator.  If we're using GCC,
Index: libiberty/configure.ac
===================================================================
RCS file: /cvs/src/src/libiberty/configure.ac,v
retrieving revision 1.22
diff -u -r1.22 configure.ac
--- libiberty/configure.ac	7 May 2005 02:00:41 -0000	1.22
+++ libiberty/configure.ac	20 May 2005 11:22:47 -0000
@@ -282,7 +282,7 @@
   sysconf times sbrk gettimeofday ffs snprintf vsnprintf \
   pstat_getstatic pstat_getdynamic sysmp getsysinfo table sysctl wait3 wait4 \
   realpath canonicalize_file_name __fsetlocking)
-  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf])
+  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf, snprintf, vsnprintf])
   AC_DEFINE(HAVE_SYS_ERRLIST, 1, [Define if you have the sys_errlist variable.])
   AC_DEFINE(HAVE_SYS_NERR,    1, [Define if you have the sys_nerr variable.])
   AC_DEFINE(HAVE_SYS_SIGLIST, 1, [Define if you have the sys_siglist variable.])
@@ -518,7 +518,7 @@
     [AC_MSG_RESULT([no])])
 
   AC_CHECK_FUNCS($checkfuncs)
-  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf])
+  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf, snprintf, vsnprintf])
   libiberty_NEED_DECLARATION(canonicalize_file_name)
 fi
 
Index: gdb/configure.ac
===================================================================
RCS file: /cvs/src/src/gdb/configure.ac,v
retrieving revision 1.18
diff -u -r1.18 configure.ac
--- gdb/configure.ac	21 Apr 2005 05:34:33 -0000	1.18
+++ gdb/configure.ac	20 May 2005 11:28:33 -0000
@@ -418,7 +418,7 @@
 
 AC_CHECK_DECLS([free, malloc, realloc])
 AC_CHECK_DECLS([strerror, strstr])
-AC_CHECK_DECLS(getopt)
+AC_CHECK_DECLS([getopt, snprintf, vsnprintf])
 
 # ----------------------- #
 # Checks for structures.  #

Re: [RFA] Eliminate warnings about snprintf declaration

[Eli Zaretskii]

> Date: Sun, 15 May 2005 13:04:35 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, gcc-patches@gcc.gnu.org,
> 	gdb-patches@sourceware.org, binutils@sourceware.org
> 
> > > gdb/ChangeLog:
> > > 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> > > 
> > > 	* configure.ac: Add snprintf and vsnprintf to AC_CHECK_DECLS.
> > 
> > I can't approve this one, but it looks fine to me.
> 
> Eli can approve it himself; or I will :-)  Same comment as above
> applies; please mention configure.

It turns out I don't know how to regenerate configure and config.in.
I looked everywhere for some dependency in some Makefile.in or for
something similar that will show me which commands are to be used for
that and in what way, but failed to find anything.  I tried to run
autoconf and autoheader, but the resultant config.in misses quite a
few undef's, like HAVE_VALUES_H and HAVE_STRCHR, so I guess I didn't
invoke it correctly.

So could someone please educate me: how should I invoke autoconf and
autoheader to produce gdb/configure and gdb/config.in, what input
files are needed (use by autoconf and autoheader) for this, and where
(in what makefile or script) are the rules to do that spelled out?

TIA

Re: [RFA] Eliminate warnings about snprintf declaration

[Andreas Schwab]

"Eli Zaretskii" <eliz@gnu.org> writes:

> So could someone please educate me: how should I invoke autoconf and
> autoheader to produce gdb/configure and gdb/config.in, what input
> files are needed (use by autoconf and autoheader) for this, and where
> (in what makefile or script) are the rules to do that spelled out?

Just make sure you use the right version of autoconf, gdb wants version
2.59.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFA] Eliminate warnings about snprintf declaration

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: gdb-patches@sourceware.org
> Date: Fri, 20 May 2005 14:42:48 +0200
> 
> Just make sure you use the right version of autoconf, gdb wants version
> 2.59.

I did verify that, and yet I still get config.in that lacks some of
the undef's it had before.

The problem is, for some uninteresting reasons, I need to regenerate
configure and config.in on a machine where I don't have the full GDB
sandbox.  I copied there configure.ac, acinclude.m4, and acconfig.h,
and invoked autoconf and autoheader, in that order.  Is this the right
procedure, or do I lack some files/commands?

Re: [RFA] Eliminate warnings about snprintf declaration

[Andreas Schwab]

"Eli Zaretskii" <eliz@gnu.org> writes:

> The problem is, for some uninteresting reasons, I need to regenerate
> configure and config.in on a machine where I don't have the full GDB
> sandbox.  I copied there configure.ac, acinclude.m4, and acconfig.h,
> and invoked autoconf and autoheader, in that order.  Is this the right
> procedure, or do I lack some files/commands?

You need aclocal.m4.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFA] Eliminate warnings about snprintf declaration

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: gdb-patches@sourceware.org
> Date: Fri, 20 May 2005 16:34:15 +0200
> 
> "Eli Zaretskii" <eliz@gnu.org> writes:
> 
> > The problem is, for some uninteresting reasons, I need to regenerate
> > configure and config.in on a machine where I don't have the full GDB
> > sandbox.  I copied there configure.ac, acinclude.m4, and acconfig.h,
> > and invoked autoconf and autoheader, in that order.  Is this the right
> > procedure, or do I lack some files/commands?
> 
> You need aclocal.m4.

Sorry, I forgot to mention: aclocal.m4 is also there.

Re: [RFA] Eliminate warnings about snprintf declaration

[Eli Zaretskii]

> Date: Fri, 20 May 2005 18:02:22 +0300
> From: "Eli Zaretskii" <eliz@gnu.org>
> CC: gdb-patches@sourceware.org
> 
> > From: Andreas Schwab <schwab@suse.de>
> > Cc: gdb-patches@sourceware.org
> > Date: Fri, 20 May 2005 16:34:15 +0200
> > 
> > "Eli Zaretskii" <eliz@gnu.org> writes:
> > 
> > > The problem is, for some uninteresting reasons, I need to regenerate
> > > configure and config.in on a machine where I don't have the full GDB
> > > sandbox.  I copied there configure.ac, acinclude.m4, and acconfig.h,
> > > and invoked autoconf and autoheader, in that order.  Is this the right
> > > procedure, or do I lack some files/commands?
> > 
> > You need aclocal.m4.
> 
> Sorry, I forgot to mention: aclocal.m4 is also there.

I dodn't know what I was missing; I ended up downloading a snapshot
and doing the work there.

The regenerated config.in and configure.ac are now committed.

Re: [RFA] Eliminate warnings about snprintf declaration

[Daniel Jacobowitz]

On Fri, May 20, 2005 at 09:18:25PM +0300, Eli Zaretskii wrote:
> I dodn't know what I was missing; I ended up downloading a snapshot
> and doing the work there.
> 
> The regenerated config.in and configure.ac are now committed.

I bet I know what it is now.  aclocal.m4 uses m4_include to include
acinclude.m4; acinclude.m4 uses sinclude to include various other
files.  Like ../bfd/bfd.m4 and ../gettext.m4.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] Eliminate warnings about snprintf declaration

[Eli Zaretskii]

> Date: Fri, 20 May 2005 14:24:25 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sourceware.org
> 
> I bet I know what it is now.  aclocal.m4 uses m4_include to include
> acinclude.m4; acinclude.m4 uses sinclude to include various other
> files.  Like ../bfd/bfd.m4 and ../gettext.m4.

Probably.

It is still a mystery to me why there's no Makefile rule to regenerate
configure and config.in when configure.ac is modified.

Re: [RFA] Eliminate warnings about snprintf declaration

[Daniel Jacobowitz]

On Fri, May 20, 2005 at 11:44:06PM +0300, Eli Zaretskii wrote:
> > Date: Fri, 20 May 2005 14:24:25 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: gdb-patches@sourceware.org
> > 
> > I bet I know what it is now.  aclocal.m4 uses m4_include to include
> > acinclude.m4; acinclude.m4 uses sinclude to include various other
> > files.  Like ../bfd/bfd.m4 and ../gettext.m4.
> 
> Probably.
> 
> It is still a mystery to me why there's no Makefile rule to regenerate
> configure and config.in when configure.ac is modified.

You can find some ranting about this topic in standards.texi, I think.
It would be inappropriate except for --enable-maintainer-mode, which
GDB doesn't seem to use much - there's only one rule for it.  Even if
we did support that, I bet the dependencies would be chronically out of
date anyway.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA] Eliminate warnings about snprintf declaration

[Andreas Schwab]

"Eli Zaretskii" <eliz@gnu.org> writes:

> It is still a mystery to me why there's no Makefile rule to regenerate
> configure and config.in when configure.ac is modified.

If gdb would use automake it would get them automatically. :-)

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFA] Eliminate warnings about snprintf declaration

[Eli Zaretskii]

> Cc: gcc-patches@gcc.gnu.org, gdb-patches@sourceware.org,
>   binutils@sourceware.org
> From: Ian Lance Taylor <ian@airs.com>
> Date: 15 May 2005 12:29:11 -0400
> 
> "Eli Zaretskii" <eliz@gnu.org> writes:
> 
> > include/ChangeLog:
> > 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> > 
> > 	* libiberty.h: (snprintf) [!HAVE_DECL_SNPRINTF]: Declare if
> > 	needed.
> > 	(vsnprintf) [!HAVE_DECL_VSNPRINTF]: Declare if needed.
> > 
> > 
> > bfd/ChangeLog:
> > 2005-05-14  Eli Zaretskii  <eliz@gnu.org>
> > 
> > 	* configure.in: Add snprintf and vsnprintf to AC_CHECK_DECLS.
> 
> These patches are approved, plus the change to libiberty/configure.ac
> which needs the obvious ChangeLog entry.  Thanks.

Thank you.

Could someone please commit these?  I don't have write access to bfd
and gcc repositories.

TIA

Re: [RFA] Eliminate warnings about snprintf declaration

[Ian Lance Taylor]

"Eli Zaretskii" <eliz@gnu.org> writes:

> > I believe you've got this logic reversed.  You also don't want or need
> > the non-prototype - it's only needed for basename because basename
> > returns a pointer.  How about this, based on the later examples in the
> > file:
> 
> Okay.  But I guess I should wait for approval by libiberty maintainers
> first.

I think Dan's comments are correct in that the patch needs to be
fixed.  I, for one, would rather approve the final version of the
patch.

The general idea is fine.

Ian