Re: gnulib: unknown option after '#pragma GCC diagnostic' ...

[Jim Meyering]

Bernhard Voelker wrote:
> Since gnulib commit 69f517e5975418e7b2c5033f8f60191919f44b9d,
> a coreutils build fails when --enable-gcc-warnings is enabled:
>
>   CC propername.o
> cc1: warnings being treated as errors
> propername.c:21:10: error: unknown option after '#pragma GCC
> diagnostic' kind [-Wpragmas]
>
> The same applies to quotearg.c:24:10 (since
> 9e62d63e086644da90db03c16907e5c7bb5a42cb).

Thanks for the report.
Fixed like this in gnulib.
I'll pull the latest gnulib into coreutils later today.

>From 2ae12d77e599965beb1483c10edb1f2f290bafc1 Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Tue, 29 Nov 2011 10:09:41 +0100
Subject: [PATCH] quotearg, propername: correct pragma guard expression

* lib/quotearg.c: Enable pragma for gcc-4.6 and newer, not 4.3 and newer.
* lib/propername.c: Likewise.  Reported by Bernhard Voelker.
---
 ChangeLog|6 ++
 lib/propername.c |2 +-
 lib/quotearg.c   |2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 28fbe90..333 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2011-11-29  Jim Meyering  
+
+   quotearg, propername: correct pragma guard expression
+   * lib/quotearg.c: Enable pragma for gcc-4.6 and newer, not 4.3 and 
newer.
+   * lib/propername.c: Likewise.  Reported by Bernhard Voelker.
+
 2011-11-28  Jim Meyering  

propername: do not mark proper_name with the const attribute
diff --git a/lib/propername.c b/lib/propername.c
index 346c310..31fc96a 100644
--- a/lib/propername.c
+++ b/lib/propername.c
@@ -17,7 +17,7 @@

 /* Without this pragma, gcc 4.7.0 2024 mistakenly suggests that
the proper_name function might be candidate for attribute 'const'  */
-#if (__GNUC__ == 4 && 3 <= __GNUC_MINOR__) || 4 < __GNUC__
+#if (__GNUC__ == 4 && 6 <= __GNUC_MINOR__) || 4 < __GNUC__
 # pragma GCC diagnostic ignored "-Wsuggest-attribute=const"
 #endif

diff --git a/lib/quotearg.c b/lib/quotearg.c
index 3c15411..03fbfe7 100644
--- a/lib/quotearg.c
+++ b/lib/quotearg.c
@@ -20,7 +20,7 @@
 /* Without this pragma, gcc 4.7.0 2024 mistakenly suggests that
the quoting_options_from_style function might be candidate for
attribute 'pure'  */
-#if (__GNUC__ == 4 && 3 <= __GNUC_MINOR__) || 4 < __GNUC__
+#if (__GNUC__ == 4 && 6 <= __GNUC_MINOR__) || 4 < __GNUC__
 # pragma GCC diagnostic ignored "-Wsuggest-attribute=pure"
 #endif

--
1.7.8.rc3.31.g017d1

Re: gnulib: unknown option after '#pragma GCC diagnostic' ...

[Bernhard Voelker]

On 11/29/2011 10:11 AM, Jim Meyering wrote:
> Bernhard Voelker wrote:
>> Since gnulib commit 69f517e5975418e7b2c5033f8f60191919f44b9d,
>> a coreutils build fails when --enable-gcc-warnings is enabled:
>>
>>   CC propername.o
>> cc1: warnings being treated as errors
>> propername.c:21:10: error: unknown option after '#pragma GCC
>> diagnostic' kind [-Wpragmas]
>>
>> The same applies to quotearg.c:24:10 (since
>> 9e62d63e086644da90db03c16907e5c7bb5a42cb).
> 
> Thanks for the report.
> Fixed like this in gnulib.
> I'll pull the latest gnulib into coreutils later today.

Thanks, it works.

Reporting with `gcc --version` output would've made live easier
for you, sorry. Just for the record, it was:
  gcc (SUSE Linux) 4.5.1 20101208 [gcc-4_5-branch revision 167585]

Have a nice day,
Berny

Re: test-float fails on ppc64 because DBL_MIN_EXP < LDBL_MIN_EXP

[Paolo Bonzini]

On 08/31/2011 05:48 PM, Jim Meyering wrote:

The test-float test is failing on ppc64 with:

 gcc version 4.4.4 20100630 (Red Hat 4.4.4-10) (GCC)
 (albeit an aging Fedora 12 system)

due to the failure of this assertion:

 ASSERT (LDBL_MIN_EXP<= DBL_MIN_EXP);

It fails because of these numbers:

 $ :|gcc -dD -E -include stddef.h -|grep -E 'L?DBL_MIN_EXP'
 #define __DBL_MIN_EXP__ (-1021)
 #define __LDBL_MIN_EXP__ (-968)


It looks like the right test would be

ASSERT (LDBL_MIN_EXP - LDBL_MANT_DIG <= DBL_MIN_EXP - DBL_MANT_DIG);

This would be enough to assert that a double can be assigned to a long 
double.


Paolo

[PATCH] hash: mark compute_bucket_size with the pure attribute

[Jim Meyering]

One more:

>From 908690cb743e69c73b42ae310807b29800c8764b Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Tue, 29 Nov 2011 14:25:56 +0100
Subject: [PATCH] hash: mark compute_bucket_size with the pure attribute

* lib/hash.c (compute_bucket_size): Use _GL_ATTRIBUTE_PURE.
---
 ChangeLog  |3 +++
 lib/hash.c |2 +-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 333..15118d9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,8 @@
 2011-11-29  Jim Meyering  

+   hash: mark compute_bucket_size with the pure attribute
+   * lib/hash.c (compute_bucket_size): Use _GL_ATTRIBUTE_PURE.
+
quotearg, propername: correct pragma guard expression
* lib/quotearg.c: Enable pragma for gcc-4.6 and newer, not 4.3 and 
newer.
* lib/propername.c: Likewise.  Reported by Bernhard Voelker.
diff --git a/lib/hash.c b/lib/hash.c
index a0e6416..1dd657a 100644
--- a/lib/hash.c
+++ b/lib/hash.c
@@ -540,7 +540,7 @@ check_tuning (Hash_table *table)
TUNING, or return 0 if there is no possible way to allocate that
many entries.  */

-static size_t
+static size_t _GL_ATTRIBUTE_PURE
 compute_bucket_size (size_t candidate, const Hash_tuning *tuning)
 {
   if (!tuning->is_n_buckets)
--
1.7.8.rc3.31.g017d1

#define FOO rpl_FOO in config.h

[John W. Eaton]

While trying to cross compile Octave for MinGW, I hit the following
errors:

  In file included from /home/jwe/src/octave/liboctave/oct-time.h:26:0,
   from /home/jwe/src/octave/liboctave/file-stat.h:28,
   from /home/jwe/src/octave/liboctave/file-ops.cc:43:
  /usr/include/c++/4.6/ctime:72:11: error: '::gmtime' has not been declared
  /usr/include/c++/4.6/ctime:73:11: error: '::localtime' has not been declared

The problem seems to be that config.h includes the lines 

  /* Define to rpl_gmtime if the replacement function should be used. */
  #define gmtime rpl_gmtime

  /* Define to rpl_localtime if the replacement function should be used. */
  #define localtime rpl_localtime

and the ctime header has

  #include 

  ...

  // Get rid of those macros defined in  in lieu of real functions.
  #undef clock
  #undef difftime
  #undef mktime
  #undef time
  #undef asctime
  #undef ctime
  #undef gmtime
  #undef localtime
  #undef strftime

  namespace std
  {
using ::clock_t;
using ::time_t;
using ::tm;

using ::clock;
using ::difftime;
using ::mktime;
using ::time;
using ::asctime;
using ::ctime;
using ::gmtime;
using ::localtime;
using ::strftime;
  } // namespace

Also, defining gmtime and localtime in config.h could cause trouble
for classes that have member functions with those names.

Surrounding the definitions in the config.h file with #ifndef
__cplusplus/#endif allowed me to get past the error, but that doesn't
seem like the right fix.

Would it be possible to handle these definitions in gnulib's time.h
instead of inserting them in config.h?

jwe

Re: #define FOO rpl_FOO in config.h

[Eric Blake]

On 11/29/2011 01:01 AM, John W. Eaton wrote:
> While trying to cross compile Octave for MinGW, I hit the following
> errors:
> 
>   In file included from /home/jwe/src/octave/liboctave/oct-time.h:26:0,
>from /home/jwe/src/octave/liboctave/file-stat.h:28,
>from /home/jwe/src/octave/liboctave/file-ops.cc:43:
>   /usr/include/c++/4.6/ctime:72:11: error: '::gmtime' has not been declared
>   /usr/include/c++/4.6/ctime:73:11: error: '::localtime' has not been declared
> 
> 
> Would it be possible to handle these definitions in gnulib's time.h
> instead of inserting them in config.h?

Yes, we need to modernize this module to get rid of the in-config.h
replacements, like we have done for other modules.  Would you like to
try helping in that process?  See commit 60b73b05 for an example of
where we did the same for mktime().

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: declare sethostname if unistd.h doesn't

[Ben Walton]

Excerpts from Simon Josefsson's message of Mon Nov 21 05:00:04 -0500 2011:

> My point was that the POSIX prototype uses size_t and Solaris seems
> to use int, and as Paul pointed out, on 64-bit Solaris systems just
> providing the prototype will break -- there needs to be a wrapper
> function or a #define that casts the second parameter, or something
> similar.  The POSIX prototype has const as well, so you would get
> warnings passing a const string to the Solaris function if there is
> no casting or wrapper function.

Sorry to be so long getting back to this.  Last week got busier than
I'd anticipated and it also took me longer to work my way through the
integration of a new module than I'd expected.

I'm at the point where I have a module that works to allow inetutils
(from bootstrap) to build hostname on Solaris (with the mismatched
types in the declaration).  This uses only the declaration from gnulib
unistd.h.  On cygwin, the sethostname function is included in libgnu.a
although I don't currently have a usable SetComputerNameEx handler and
rely on -1/ENOSYS there still.  This is on my todo list still.

What I'm not sure about now is the gnulib way to provide a declaration
that will be valid for Solaris.  Is it better to simply ifdef the
declaration in unistd.h to be Solaris specific or is an alternate
wrapper function the way to go?  If an alternate function is better,
what's the best mechanism to handle this?  Is this a case where
REPLACE_SETHOSTNAME would be warranted?

If it would help and people wouldn't mind, I'll post the current patch
series for review so that if I'm off track I can correct before
implementing the remaining bits (including the tests).

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Paul Eggert]

On 11/29/11 09:35, Jim Meyering wrote:
> +  gl_WARN_ADD([-Wno-unsuffixed-float-constants])

How about if we remove -Wunsuffixed-float-constants from
manywarnings.m4?  In practice it typically causes more
trouble than it cures (the above-quoted gzip patch is
one example, but I've run into it elsewhere).

While we're on the subject of manywarnings.m4, can we also
remove -Wdouble-promotion and -Wformat-zero-length?  They
also seem to be more suited for special- rather than
general-purpose code.

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Jim Meyering]

Paul Eggert wrote:
> On 11/29/11 09:35, Jim Meyering wrote:
>> +  gl_WARN_ADD([-Wno-unsuffixed-float-constants])
>
> How about if we remove -Wunsuffixed-float-constants from
> manywarnings.m4?  In practice it typically causes more
> trouble than it cures (the above-quoted gzip patch is
> one example, but I've run into it elsewhere).

Good idea.

> While we're on the subject of manywarnings.m4, can we also
> remove -Wdouble-promotion and -Wformat-zero-length?  They
> also seem to be more suited for special- rather than
> general-purpose code.

Coreutils' gnulib lib/* files had only one
"-Wdouble-promotion"-triggered warning:

# FIXME: it may be easy to remove this, since it affects only one file:
# the snprintf call at ftoastr.c:132.
nw="$nw -Wdouble-promotion"

So maybe it's not a burden to most projects.
I haven't investigated the ftoastr.c issue.

Have you found code that triggers a -Wformat-zero-length warning
yet that doesn't seem worth adjusting?

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Eric Blake]

On 11/29/2011 02:05 PM, Paul Eggert wrote:
> On 11/29/11 09:35, Jim Meyering wrote:
>> +  gl_WARN_ADD([-Wno-unsuffixed-float-constants])
> 
> How about if we remove -Wunsuffixed-float-constants from
> manywarnings.m4?  In practice it typically causes more
> trouble than it cures (the above-quoted gzip patch is
> one example, but I've run into it elsewhere).

I've seen -Wunsuffixed-float-constants trigger in a couple projects now,
so it really is a new warning that pops up on existing code; but it's
also fairly mechanical to correct (for example, while fixing it for
libvirt, I found at least one case where it was sufficient to use int
instead of floating point).

I'm not convinced about removing it from manywarnings.m4 - it's not that
hard to disable the warning if you don't want it, but leaving it in
leads to smaller executable size for the cases where 1.0F is sufficient
(compared to the extra size required to represent 1.0 which is 1.0D).
By explicitly marking F or D to all float constants, it shows you've
thought about which precision is worth using; omitting the suffix could
be the sign of sloppy code that has other problems with misuse of
floating point.

> 
> While we're on the subject of manywarnings.m4, can we also
> remove -Wdouble-promotion and -Wformat-zero-length?  They
> also seem to be more suited for special- rather than
> general-purpose code.

Do you have instances in the wild where these triggered?  I haven't seen
either one trigger, and it's hard to say that removing it is worthwhile
without an idea of what it is protecting against, and whether the
protection flies in the face of common programming idioms to actually
make the warning useful only in specific coding styles.

Meanwhile, while we are on the topic, I got tripped up by the fact that
-Wformat=2 (newly added to manywarnings.m4) implies -Wformat-nonliteral,
even though libvirt had already been explicitly removing
-Wformat-nonliteral from the list of desired warnings.  I had to add
-Wno-format-nonliteral to counteract that, while still benefitting from
the rest of -Wformat=2.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Paul Eggert]

On 11/29/11 13:18, Jim Meyering wrote:
> Have you found code that triggers a -Wformat-zero-length warning
> yet that doesn't seem worth adjusting?

I haven't run into it one way or another, but all my instincts are
against that diagnostic.  I suspect that the most-common way that it
would happen would be something like this admittedly-contrived example:

   #if FEATURE_ENABLED
#define FEATURE_FORMAT "feature"
   #else
#define FEATURE_FORMAT ""
   #endif
   ...
   printf (buf, FEATURE_FORMAT);

where the printf can be optimized away if FEATURE_ENABLED
is zero, and the compiler is warning us about that.
This reminds me too much about bogus warnings that some compilers
give for this:

   enum { N = FEATURE_ENABLED ? 1000 : 0 };
   ...
   for (i = 0; i < N; i++)
 foo (i);

where the compiler proudly warns that it has optimized the loop
away entirely, and did I really mean that?  (Yes I did! and I don't
want to be warned about it! :-)

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Eric Blake]

On 11/29/2011 02:27 PM, Paul Eggert wrote:
> On 11/29/11 13:18, Jim Meyering wrote:
>> Have you found code that triggers a -Wformat-zero-length warning
>> yet that doesn't seem worth adjusting?
> 
> I haven't run into it one way or another, but all my instincts are
> against that diagnostic.  I suspect that the most-common way that it
> would happen would be something like this admittedly-contrived example:
> 
>#if FEATURE_ENABLED
> #define FEATURE_FORMAT "feature"
>#else
> #define FEATURE_FORMAT ""
>#endif
>...
>printf (buf, FEATURE_FORMAT);

I assume you meant sprintf (buf, FEATURE_FORMAT), or printf
(FEATURE_FORMAT)?

> 
> where the printf can be optimized away if FEATURE_ENABLED
> is zero, and the compiler is warning us about that.

Ah, but then I would use these more efficient idioms, which also avoids
the compiler warning:

puts (FEATURE_FORMAT) (replacing the printf case)
strcpy (buf, FEATURE_FORMAT) (replacing the sprintf case)

> This reminds me too much about bogus warnings that some compilers
> give for this:
> 
>enum { N = FEATURE_ENABLED ? 1000 : 0 };
>...
>for (i = 0; i < N; i++)
>  foo (i);
> 
> where the compiler proudly warns that it has optimized the loop
> away entirely, and did I really mean that?  (Yes I did! and I don't
> want to be warned about it! :-)

I agree that a noisy compiler telling you about dead code due to
optimizations is a pain, but until we actually encounter the noise, I
can't say whether the signal-to-noise ratio warrants removing the
warning by default.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Jim Meyering]

Eric Blake wrote:

> On 11/29/2011 02:05 PM, Paul Eggert wrote:
>> On 11/29/11 09:35, Jim Meyering wrote:
>>> +  gl_WARN_ADD([-Wno-unsuffixed-float-constants])
>>
>> How about if we remove -Wunsuffixed-float-constants from
>> manywarnings.m4?  In practice it typically causes more
>> trouble than it cures (the above-quoted gzip patch is
>> one example, but I've run into it elsewhere).
>
> I've seen -Wunsuffixed-float-constants trigger in a couple projects now,
> so it really is a new warning that pops up on existing code; but it's
> also fairly mechanical to correct (for example, while fixing it for
> libvirt, I found at least one case where it was sufficient to use int
> instead of floating point).
>
> I'm not convinced about removing it from manywarnings.m4 - it's not that
> hard to disable the warning if you don't want it, but leaving it in
> leads to smaller executable size for the cases where 1.0F is sufficient
> (compared to the extra size required to represent 1.0 which is 1.0D).
> By explicitly marking F or D to all float constants, it shows you've
> thought about which precision is worth using; omitting the suffix could
> be the sign of sloppy code that has other problems with misuse of
> floating point.

Inspired by that, I went to see what would be required for coreutils to pass.
Are these "D" and "F" really worth it?

Unless there are objections (portability?)
I'll fix at least the affected header files: intprops.h, timespec.h.
At which point, I might as well fix the others, too.
There aren't many, considering the amount of code.

First the gnulib changes:

diff --git a/lib/hash.c b/lib/hash.c
index 1dd657a..9b6e4db 100644
--- a/lib/hash.c
+++ b/lib/hash.c
@@ -113,8 +113,8 @@ struct hash_table
1.0).  The growth threshold defaults to 0.8, and the growth factor
defaults to 1.414, meaning that the table will have doubled its size
every second time 80% of the buckets get used.  */
-#define DEFAULT_GROWTH_THRESHOLD 0.8
-#define DEFAULT_GROWTH_FACTOR 1.414
+#define DEFAULT_GROWTH_THRESHOLD 0.8F
+#define DEFAULT_GROWTH_FACTOR 1.414F

 /* If a deletion empties a bucket and causes the ratio of used buckets to
table size to become smaller than the shrink threshold (a number between
@@ -122,8 +122,8 @@ struct hash_table
number greater than the shrink threshold but smaller than 1.0).  The shrink
threshold and factor default to 0.0 and 1.0, meaning that the table never
shrinks.  */
-#define DEFAULT_SHRINK_THRESHOLD 0.0
-#define DEFAULT_SHRINK_FACTOR 1.0
+#define DEFAULT_SHRINK_THRESHOLD 0.0F
+#define DEFAULT_SHRINK_FACTOR 1.0F

 /* Use this to initialize or reset a TUNING structure to
some sensible values. */
@@ -238,7 +238,7 @@ hash_print_statistics (const Hash_table *table, FILE 
*stream)
   fprintf (stream, "# buckets: %lu\n", (unsigned long int) n_buckets);
   fprintf (stream, "# buckets used:%lu (%.2f%%)\n",
(unsigned long int) n_buckets_used,
-   (100.0 * n_buckets_used) / n_buckets);
+   (100.0F * n_buckets_used) / n_buckets);
   fprintf (stream, "max bucket length: %lu\n",
(unsigned long int) max_bucket_length);
 }
diff --git a/lib/intprops.h b/lib/intprops.h
index 1f6a539..7bce5db 100644
--- a/lib/intprops.h
+++ b/lib/intprops.h
@@ -35,7 +35,7 @@

 /* True if the arithmetic type T is an integer type.  bool counts as
an integer.  */
-#define TYPE_IS_INTEGER(t) ((t) 1.5 == 1)
+#define TYPE_IS_INTEGER(t) ((t) 1.5F == 1)

 /* True if negative values of the signed integer type T use two's
complement, ones' complement, or signed magnitude representation,
diff --git a/lib/timespec.h b/lib/timespec.h
index acf815c..6b783e3 100644
--- a/lib/timespec.h
+++ b/lib/timespec.h
@@ -73,7 +73,7 @@ struct timespec dtotimespec (double);
 static inline double
 timespectod (struct timespec a)
 {
-  return a.tv_sec + a.tv_nsec / 1e9;
+  return a.tv_sec + a.tv_nsec / 1e9D;
 }

 void gettime (struct timespec *);

===
And then the coreutils diffs:

diff --git a/configure.ac b/configure.ac
index 8c4fff4..9ca2ce4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -122,7 +122,6 @@ if test "$gl_gcc_warnings" = yes; then
   gl_WARN_ADD([-Wsuggest-attribute=const])
   gl_WARN_ADD([-Wsuggest-attribute=noreturn])
   gl_WARN_ADD([-Wno-format-nonliteral])
-  gl_WARN_ADD([-Wno-unsuffixed-float-constants])

   # Enable this warning only with gcc-4.7 and newer.  With 4.6.2 20111027,
   # it suggests test.c's advance function may be pure, even though it
diff --git a/src/sleep.c b/src/sleep.c
index d32daa4..70e5deb 100644
--- a/src/sleep.c
+++ b/src/sleep.c
@@ -100,7 +100,7 @@ int
 main (int argc, char **argv)
 {
   int i;
-  double seconds = 0.0;
+  double seconds = 0.0D;
   bool ok = true;

   initialize_main (&argc, &argv);
diff --git a/src/sort.c b/src/sort.c
index 4a87148..02bf89f 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -987,7 +987,7 @@ pipe_fork (int pipefds[2], size_t tries

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Paul Eggert]

On 11/29/11 13:19, Eric Blake wrote:

> hard to disable the warning if you don't want it, but leaving it in
> leads to smaller executable size for the cases where 1.0F is sufficient
> (compared to the extra size required to represent 1.0 which is 1.0D).

1.0D?  But the C standard doesn't allow that.  Surely such a constant
can't be used in portable code.  So I don't see how the warning can
be useful in practice.

>> While we're on the subject of manywarnings.m4, can we also
>> remove -Wdouble-promotion and -Wformat-zero-length?  They
>> also seem to be more suited for special- rather than
>> general-purpose code.
> 
> Do you have instances in the wild where these triggered?

No, it's just my instinct -- I gave an example for -Wformat-zero-length
in my earlier email, and my instinct also is that widening float to
double is no more problematic than widening int to long (which I
surely don't want a warning for).

> I had to add -Wno-format-nonliteral to counteract that, while still
> benefitting from the rest of -Wformat=2.

I'd be in favor of that as well.  format-nonliteral is a warning that
typically causes more harm than it cures, in my experience.

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Eric Blake]

On 11/29/2011 02:38 PM, Jim Meyering wrote:
>> I'm not convinced about removing it from manywarnings.m4 - it's not that
>> hard to disable the warning if you don't want it, but leaving it in
>> leads to smaller executable size for the cases where 1.0F is sufficient
>> (compared to the extra size required to represent 1.0 which is 1.0D).
>> By explicitly marking F or D to all float constants, it shows you've
>> thought about which precision is worth using; omitting the suffix could
>> be the sign of sloppy code that has other problems with misuse of
>> floating point.
> 
> Inspired by that, I went to see what would be required for coreutils to pass.
> Are these "D" and "F" really worth it?
> 
> Unless there are objections (portability?)

Aargh.  I just reread C99.

F (and f) for float, and L (or l) for long double are required, but D
(or d) for double is a GNU extension.

Since we can't silence the warning without adding an explicit 'D', but
'D' is not standardized, I have changed my mind.  Let's nuke the warning.

Meanwhile, your patch for adding 'F' is okay, but not for adding 'D'.
That is,

> +++ b/lib/hash.c
> @@ -113,8 +113,8 @@ struct hash_table
> 1.0).  The growth threshold defaults to 0.8, and the growth factor
> defaults to 1.414, meaning that the table will have doubled its size
> every second time 80% of the buckets get used.  */
> -#define DEFAULT_GROWTH_THRESHOLD 0.8
> -#define DEFAULT_GROWTH_FACTOR 1.414
> +#define DEFAULT_GROWTH_THRESHOLD 0.8F
> +#define DEFAULT_GROWTH_FACTOR 1.414F

this change makes sense,

> @@ -238,7 +238,7 @@ hash_print_statistics (const Hash_table *table, FILE 
> *stream)
>fprintf (stream, "# buckets: %lu\n", (unsigned long int) 
> n_buckets);
>fprintf (stream, "# buckets used:%lu (%.2f%%)\n",
> (unsigned long int) n_buckets_used,
> -   (100.0 * n_buckets_used) / n_buckets);
> +   (100.0F * n_buckets_used) / n_buckets);

but this does not (in var-args, float gets promoted to double, so you
probably aren't gaining anything by using 'float' as an intermediary,
and starting with '100.0' as double is better to begin with).

> @@ -73,7 +73,7 @@ struct timespec dtotimespec (double);
>  static inline double
>  timespectod (struct timespec a)
>  {
> -  return a.tv_sec + a.tv_nsec / 1e9;
> +  return a.tv_sec + a.tv_nsec / 1e9D;

Likewise, this one is not portable.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Paul Eggert]

On 11/29/11 13:38, Jim Meyering wrote:

> -#define TYPE_IS_INTEGER(t) ((t) 1.5 == 1)
> +#define TYPE_IS_INTEGER(t) ((t) 1.5F == 1)

I'd rather omit this.  The constant is represented exactly and is an
immediate operand of a cast.  (And I wouldn't be surprised if some
compilers warned about the "F" for that reason)  If GCC is warning
about this, that's more of a GCC bug than anything else.

I second Eric's other comments about the changes.

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Jim Meyering]

Paul Eggert wrote:
> On 11/29/11 13:38, Jim Meyering wrote:
>
>> -#define TYPE_IS_INTEGER(t) ((t) 1.5 == 1)
>> +#define TYPE_IS_INTEGER(t) ((t) 1.5F == 1)
>
> I'd rather omit this.  The constant is represented exactly and is an
> immediate operand of a cast.  (And I wouldn't be surprised if some
> compilers warned about the "F" for that reason)  If GCC is warning
> about this, that's more of a GCC bug than anything else.
>
> I second Eric's other comments about the changes.

I agree.

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Eric Blake]

On 11/29/2011 02:46 PM, Eric Blake wrote:
>> Unless there are objections (portability?)
> 
> Aargh.  I just reread C99.
> 
> F (and f) for float, and L (or l) for long double are required, but D
> (or d) for double is a GNU extension.
> 
> Since we can't silence the warning without adding an explicit 'D', but
> 'D' is not standardized, I have changed my mind.  Let's nuke the warning.

So for starters, I'm pushing this:

From b84c90b17e947a5140857d646c7ed7396c129898 Mon Sep 17 00:00:00 2001
From: Eric Blake 
Date: Tue, 29 Nov 2011 15:01:22 -0700
Subject: [PATCH] manywarnings: drop -Wunsuffixed-float-constants

* m4/manywarnings.m4 (gl_MANYWARN_ALL_GCC): C99 does not allow
'1.0D', which is the only way to silence this warning for 'double'.

Signed-off-by: Eric Blake 
---
 ChangeLog  |4 
 m4/manywarnings.m4 |3 +--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index f90fd61..48857f5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
 2011-11-29  Eric Blake  

+   manywarnings: drop -Wunsuffixed-float-constants
+   * m4/manywarnings.m4 (gl_MANYWARN_ALL_GCC): C99 does not allow
+   '1.0D', which is the only way to silence this warning for 'double'.
+
maint.mk: add syntax check for use of compare from init.sh
* top/maint.mk (sc_prohibit_reversed_compare_failure): New rule,
moved here from coreutils.
diff --git a/m4/manywarnings.m4 b/m4/manywarnings.m4
index 6e78c07..23bc61c 100644
--- a/m4/manywarnings.m4
+++ b/m4/manywarnings.m4
@@ -1,4 +1,4 @@
-# manywarnings.m4 serial 1
+# manywarnings.m4 serial 2
 dnl Copyright (C) 2008-2011 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -171,7 +171,6 @@ AC_DEFUN([gl_MANYWARN_ALL_GCC],
 -Wsuggest-attribute=noreturn \
 -Wsuggest-attribute=pure \
 -Wtrampolines \
--Wunsuffixed-float-constants \
 ; do
 gl_manywarn_set="$gl_manywarn_set $gl_manywarn_item"
   done
-- 
1.7.7.3



-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Paul Eggert]

On 11/29/11 13:32, Eric Blake wrote:
>>#if FEATURE_ENABLED
>> > #define FEATURE_FORMAT "feature"
>> >#else
>> > #define FEATURE_FORMAT ""
>> >#endif
>> >...
>> >printf (buf, FEATURE_FORMAT);
> I assume you meant sprintf (buf, FEATURE_FORMAT), or printf
> (FEATURE_FORMAT)?

Yes, that's right.

Since my earlier example wasn't convincing enough, how
about this one?

#if FEATURE_ENABLED
 #define FEATURE_FORMAT_ARGS(a, b) "(%d, %s)" a, b
#else
 #define FEATURE_FORMAT_ARGS(a, b) ""
#endif
...
printf (FEATURE_FORMAT_ARGS (i, argv[i]);
...
snprintf (buf, sizeof buf, FEATURE_FORMAT_ARGS (i, argv[i]);

I wouldn't mind so much if I thought that -Wformat-zero-length
would be useful, but I can't think of a single likely
use case for it.  It's not like there's a real problem with
people writing 'printf ("");' when they should write nothing.

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Jim Meyering]

Eric Blake wrote:
> On 11/29/2011 02:38 PM, Jim Meyering wrote:
...
> Meanwhile, your patch for adding 'F' is okay, but not for adding 'D'.
> That is,
>
>> +++ b/lib/hash.c
>> @@ -113,8 +113,8 @@ struct hash_table
>> 1.0).  The growth threshold defaults to 0.8, and the growth factor
>> defaults to 1.414, meaning that the table will have doubled its size
>> every second time 80% of the buckets get used.  */
>> -#define DEFAULT_GROWTH_THRESHOLD 0.8
>> -#define DEFAULT_GROWTH_FACTOR 1.414
>> +#define DEFAULT_GROWTH_THRESHOLD 0.8F
>> +#define DEFAULT_GROWTH_FACTOR 1.414F
>
> this change makes sense,
>
>> @@ -238,7 +238,7 @@ hash_print_statistics (const Hash_table *table, FILE 
>> *stream)
>>fprintf (stream, "# buckets: %lu\n", (unsigned long int) 
>> n_buckets);
>>fprintf (stream, "# buckets used:%lu (%.2f%%)\n",
>> (unsigned long int) n_buckets_used,
>> -   (100.0 * n_buckets_used) / n_buckets);
>> +   (100.0F * n_buckets_used) / n_buckets);
>
> but this does not (in var-args, float gets promoted to double, so you
> probably aren't gaining anything by using 'float' as an intermediary,
> and starting with '100.0' as double is better to begin with).

Actually, if we cared about avoiding the warning, F would be fine there,
since it's printing to a mere %.2f format.  We certainly don't need all
of double's precision or exponent range for that.

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Eric Blake]

On 11/29/2011 03:23 PM, Jim Meyering wrote:
>>> @@ -238,7 +238,7 @@ hash_print_statistics (const Hash_table *table, FILE 
>>> *stream)
>>>fprintf (stream, "# buckets: %lu\n", (unsigned long int) 
>>> n_buckets);
>>>fprintf (stream, "# buckets used:%lu (%.2f%%)\n",
>>> (unsigned long int) n_buckets_used,
>>> -   (100.0 * n_buckets_used) / n_buckets);
>>> +   (100.0F * n_buckets_used) / n_buckets);
>>
>> but this does not (in var-args, float gets promoted to double, so you
>> probably aren't gaining anything by using 'float' as an intermediary,
>> and starting with '100.0' as double is better to begin with).
> 
> Actually, if we cared about avoiding the warning, F would be fine there,
> since it's printing to a mere %.2f format.  We certainly don't need all
> of double's precision or exponent range for that.
> 

You missed my point - the compiler is going to be converting float to
double to pass the argument to fprintf, and fprintf will be operating on
double (%f and %lf both mean double on printf; they only differ on float
vs. double when dealing with scanf).  I see your point about the
possibility of the intermediate division in float possibly being faster,
but no matter how we look at things, the end result is still fprintf
operating on double, not float, whether or not we cared about the extra
precision.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[PATCH] maint.mk: add syntax check for use of compare from init.sh

[Eric Blake]

Comparing expected against actual gives more consistent diff listings
when reporting test failures.  Enforce this idiom on test files
that use init.sh, and allow projects to recognize an alternate
pattern for recognizing tests scripts that use a compare function.

* top/maint.mk (sc_prohibit_reversed_compare_failure): New rule,
moved here from coreutils.

Signed-off-by: Eric Blake 
---

>> You're welcome to remove it from coreutils' cfg.mk, too.
> 
> Okay, I'm now in the middle of coding up the move, and will post and
> push the patches if results look reasonable with libvirt.

Here's what I'm pushing to gnulib.  I tested it with libvirt, where
I had to add an override in cfg.mk to detect the problems:
 _test_script_regex = \<\(init\|test-lib\)\.sh\>

It threw me a bit that $prohibit is an ERE, but $containing is a BRE.
I wonder if we should fix $(_sc_search_regexp) to consistently use
ERE, but it would be an incompatible change for existing checks.

I also tested with grep and coreutils, at points both before and after
your recent global cleanups, to prove that it caught the problems and
that the problem is now fixed.  A slight tweak to the forbid pattern
is handy for actual cases I found in libvirt by manual inspection,
which had uses like 'compare "$actual" "$exp"' and 'compare file
file-exp'.  That tweak also exposed two places missed in coreutils,
which I will fix in the next patch to coreutils.

 ChangeLog|4 
 top/maint.mk |   10 ++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 95d71fd..9735aa8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
 2011-11-29  Eric Blake  

+   maint.mk: add syntax check for use of compare from init.sh
+   * top/maint.mk (sc_prohibit_reversed_compare_failure): New rule,
+   moved here from coreutils.
+
manywarnings: drop -Wunsuffixed-float-constants
* m4/manywarnings.m4 (gl_MANYWARN_ALL_GCC): C99 does not allow
'1.0D', which is the only way to silence this warning for 'double'.
diff --git a/top/maint.mk b/top/maint.mk
index 76844a0..e1375df 100644
--- a/top/maint.mk
+++ b/top/maint.mk
@@ -1157,6 +1157,16 @@ sc_cross_check_PATH_usage_in_tests:
1>&2; exit 1; } || :;   \
fi

+# BRE regex of file contents to identify a test script.
+_test_script_regex ?= \
+
+# In tests, use "compare expected actual", not the reverse.
+sc_prohibit_reversed_compare_failure:
+   @prohibit='\

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Bruno Haible]

Eric Blake wrote:
> Meanwhile, your patch for adding 'F' is okay

Why in upper case? The common habit seems to be to use the suffix 'f'
in lower case but 'L' in upper case. Don't ask me why, but this is the
way it's done in the vast majority of the glibc sources.

Bruno
-- 
In memoriam Willy Cohn 

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Eric Blake]

On 11/29/2011 04:50 PM, Bruno Haible wrote:
> Eric Blake wrote:
>> Meanwhile, your patch for adding 'F' is okay
> 
> Why in upper case? The common habit seems to be to use the suffix 'f'
> in lower case but 'L' in upper case. Don't ask me why, but this is the
> way it's done in the vast majority of the glibc sources.

I guess the argument here is that 'f' is easier to type, and still
visually distinct, while 'l' looks too much like '1' so 'L' helps make
the distinction.  Also, %Lg is how you specify long double to printf, so
writing a constant with L helps.

No problem by me if we stick to 'f' and 'L' for the places where we want
suffixes.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: posix_memalign

[Bruno Haible]

Eric Blake wrote:
> > We have a 'pagealign_alloc' module that does not waste memory.
> 
> Alas, pagealign_alloc is currently GPLv3+, although if libvirt were to
> use it in place of posix_memalign, it would have to be LGPLv2+.  It
> looks like Jim, Paul, and Bruno are the only contributors so far.

Don't forget Derek Robert Price, who initially wrote this code.

> How important is it that our special-case allocation/free in
> pagealign_alloc() remain GPL?

Ask the 4 copyright holders.

> > I wouldn't like to slow down free(), which is used in many places, for the
> > sake of posix_memalign() (as opposed to pagealign_alloc()) which is rarely
> > used.
> 
> It would only be slowed down on systems where we cannot otherwise get
> aligned allocations.  Per your research:
> 
> Minix 3.1.8   not page-aligned at all
> AIX 5.1   not page-aligned at all
> HP-UX 11  not page-aligned at all
> IRIX 6.5  not page-aligned at all
> OSF/1 5.1 not page-aligned at all
> Solaris 10not page-aligned at all
> Cygwin 1.5.x  not page-aligned at all
> mingw not page-aligned at all
> MSVC 9not page-aligned at all

This is a whole lot of systems!

> Or maybe we introduce an
> LGPLv2+ posix_memalign that wastefully overallocates and slows down
> free(), and leave pagealign_alloc() as GPLv3+, as a way to encourage
> licensing improvements for people that care about efficiency on
> deficient platforms.

If the issue with the license is that big, please provide a posix_memalign-
like function that has its own _free function, under LGPLv2+.

I don't want to hear reports like "my program got 5% slower after I started
using gnulib module xyz". Overriding free() to do nontrivial stuff has the
potential for a 5% slowdown. If we can't provide a POSIX API portably, let's
instead choose the most similar possible API.

> Thinking aloud here, another possibility might be to use mmap() to
> provide posix_memalign() at page boundaries, as well as reserving the
> previous page as an unmapped guard page.  Most implementations of
> malloc() have a free() that assumes a header was present immediately
> before the pointer returned by malloc(), and would thus fault in free()
> while trying to access the header just before a pointer returned by
> posix_memalign. ...

Nice trick, but unfortunately not usable for functions that are meant to
be called from libraries. Signal handling has global impact on a program.

Bruno
-- 
In memoriam Willy Cohn 

Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Paul Eggert]

On 11/29/11 14:23, Jim Meyering wrote:
> if we cared about avoiding the warning, F would be fine there,
> since it's printing to a mere %.2f format.  We certainly don't need all
> of double's precision or exponent range for that.

For that app you're right, accuracy doesn't matter.
But in general I avoid 'float' for stuff like that.
E.g., if n_buckets_used is 1326 and n_buckets is 1583,

   printf ("%.2f", (100.0F * n_buckets_used) / n_buckets);

outputs the wrong answer, whereas omitting the F causes
it to output the right answer (this is on x86-64 compiled
with gcc 4.6.2 -O2).  Also, the code with 'float' is
slightly bigger and presumably a bit slower (this is
a common phenomenon on x86 and x86-64).

It's only the last digit that's wrong, of course,
and the performance difference is tiny,
but it's frustrating that some developer went
to the work of putting in that F to make the answer
slightly slower and slightly wrong.

Re: undefined behavior in hol_append()

[Bruno Haible]

Paul Eggert wrote:
> > Do you know of any platforms where sizeof (ptrdiff_t) < sizeof (void *) ?
> 
> The standard operating mode for ILE C/C++ (for IBM i)
> has 16-byte pointers; I expect that sizeof (ptrdiff_t)
> is 8 and sizeof (void *) is 16 on such platforms, though
> I don't have a short citation supporting this.

Thanks for the reference. This is a platform that's in use today. So I'm
applying Matthew's suggestion.

Matthew, the "(tiny change)" annotation is not meant to be derogatory. It
simply states that no exchange of legal paperwork was needed for integrating
your patch.


2011-11-29  Matthew Wala(tiny change)

Avoid subtracting two pointers that don't point into the same block.
* lib/argp-help.c (hol_append): Reorder pointer subtractions so that
only pointers into the same memory block are subtracted. We cannot
assume that sizeof (ptrdiff_t) == sizeof (void *).

--- lib/argp-help.c.origWed Nov 30 01:39:20 2011
+++ lib/argp-help.c Wed Nov 30 01:37:25 2011
@@ -891,7 +891,8 @@
 
   /* Fix up the short options pointers from HOL.  */
   for (e = entries, left = hol->num_entries; left > 0; e++, left--)
-e->short_options += (short_options - hol->short_options);
+e->short_options =
+  short_options + (e->short_options - hol->short_options);
 
   /* Now add the short options from MORE, fixing up its entries
  too.  */

-- 
In memoriam Willy Cohn 

Re: test-float fails on ppc64 because DBL_MIN_EXP < LDBL_MIN_EXP

[Bruno Haible]

Paolo Bonzini wrote:
> > due to the failure of this assertion:
> >
> >  ASSERT (LDBL_MIN_EXP<= DBL_MIN_EXP);
> >
> > It fails because of these numbers:
> >
> >  $ :|gcc -dD -E -include stddef.h -|grep -E 'L?DBL_MIN_EXP'
> >  #define __DBL_MIN_EXP__ (-1021)
> >  #define __LDBL_MIN_EXP__ (-968)
> 
> It looks like the right test would be
> 
> ASSERT (LDBL_MIN_EXP - LDBL_MANT_DIG <= DBL_MIN_EXP - DBL_MANT_DIG);

> This would be enough to assert that a double can be assigned to a long 
> double.

[without loss of accuracy]. Indeed, thanks Paolo for the nice fix. I've
applied it for you, like this:


2011-11-29  Paolo Bonzini  

float tests: Correct and re-enable assertion about LDBL_MIN_EXP.
* tests/test-float.c (test_long_double): Correct and re-enable the
assertion about LDBL_MIN_EXP that was disabled on 2011-08-31.

--- tests/test-float.c.orig Wed Nov 30 01:58:31 2011
+++ tests/test-float.c  Wed Nov 30 01:52:03 2011
@@ -298,14 +298,7 @@
 
   /* Check that 'long double' is at least as wide as 'double'.  */
   ASSERT (LDBL_MANT_DIG >= DBL_MANT_DIG);
-
-  /* Normally, we would also assert this:
-   ASSERT (LDBL_MIN_EXP <= DBL_MIN_EXP);
- but at least on powerpc64 with gcc-4.4.4, it would fail:
- $ :|gcc -dD -E -include stddef.h -|grep -E 'L?DBL_MIN_EXP'
- #define __DBL_MIN_EXP__ (-1021)
- #define __LDBL_MIN_EXP__ (-968)
-  */
+  ASSERT (LDBL_MIN_EXP - LDBL_MANT_DIG <= DBL_MIN_EXP - DBL_MANT_DIG);
   ASSERT (LDBL_MAX_EXP >= DBL_MAX_EXP);
 
   /* Check the value of LDBL_DIG.  */
-- 
In memoriam Willy Cohn 

Re: declare sethostname if unistd.h doesn't

[Bruno Haible]

Hello Ben,

> If it would help and people wouldn't mind, I'll post the current patch
> series for review so that if I'm off track I can correct before
> implementing the remaining bits (including the tests).

Cool. Yes, please, show it!

> On cygwin, the sethostname function is included in libgnu.a
> although I don't currently have a usable SetComputerNameEx handler and
> rely on -1/ENOSYS there still.  This is on my todo list still.

It can also be done later by someone else who is more familiar with
Win32 programming. We prefer a module with missing mingw or Cygwin
support to a module that does not exist at all.

> What I'm not sure about now is the gnulib way to provide a declaration
> that will be valid for Solaris.  Is it better to simply ifdef the
> declaration in unistd.h to be Solaris specific or is an alternate
> wrapper function the way to go?

Good question. Let's see the facts:
  - On Solaris 11,  declares sethostname(), with a second parameter
of type 'int'.
  - But we want the same prototype on all platforms, that is, 'size_t' as
second parameter.
  - The question is therefore: Is it dangerous to pass a 'size_t' value
to a function that originally was defined with an 'int' parameter?
Negative values of values beyond INT_MAX make no sense, since the
parameter denotes the length of a string.
  - So, considering only how nonnegative values are passed.
On 32-bit platforms, 'size_t' is 'unsigned int', which is not
distinguishable from 'int' in the ABI.
On 64-bit platforms, that is - for Solaris - SPARC64 and x86_64, the first
two arguments (and more) are passed in 64-bit registers, not on the stack.
Passing a 64-bit 'size_t' in place of a 32-bit 'int' is therefore OK.

In summary, here you don't need the wrapper function when the function is not
already declared.

If it is declared and has the wrong parameter type, you need a wrapper. See
e.g. in gettimeofday.c or ioctl.c for how we write such wrappers. This is the
case where you set REPLACE_SETHOSTNAME to 1.

Bruno
-- 
In memoriam Willy Cohn 

[PATCH 1/3] Split the HOST_NAME_MAX detection into separate m4 macro

[Ben Walton]

The sethostname module will rely on this code too, so make it a
separate function.

Signed-off-by: Ben Walton 
---
 m4/gethostname.m4 |6 +-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/m4/gethostname.m4 b/m4/gethostname.m4
index 8ea6329..784e40a 100644
--- a/m4/gethostname.m4
+++ b/m4/gethostname.m4
@@ -1,4 +1,4 @@
-# gethostname.m4 serial 12
+# gethostname.m4 serial 13
 dnl Copyright (C) 2002, 2008-2011 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -40,6 +40,10 @@ AC_DEFUN([gl_FUNC_GETHOSTNAME],
 HAVE_GETHOSTNAME=0
   fi
 
+  gl_PREREQ_HOST_NAME_MAX
+])
+
+AC_DEFUN([gl_PREREQ_HOST_NAME_MAX], [
   dnl Also provide HOST_NAME_MAX when  lacks it.
   dnl - On most Unix systems, use MAXHOSTNAMELEN from  instead.
   dnl - On Solaris, Cygwin, BeOS, use MAXHOSTNAMELEN from  instead.
-- 
1.7.4.1

[RFC] sethostname handling patch series

[Ben Walton]

Hi Bruno,

> Cool. Yes, please, show it!

Ok, it's following this reply.  I'm sure that it's not ready for
inclusion just yet, but I'll polish it until it is using the feedback
I receive.  As always, comments and criticisms welcomed.

> It can also be done later by someone else who is more familiar with
> Win32 programming. We prefer a module with missing mingw or Cygwin
> support to a module that does not exist at all.

Ok, that would be fine with me too.

> In summary, here you don't need the wrapper function when the
> function is not already declared.

Thanks for that breakdown.  It was very enlightening.

Thanks
-Ben

[PATCH 2/3] Add a new sethostname module

[Ben Walton]

Define sethostname on platforms that do not provide the declaration.
Provide a function for platforms that lack it.  The general handling
of the provided function is to simply return -1 and set errno to
ENOSYS.  A handler is provided for Minix.

Signed-off-by: Ben Walton 
---
 doc/glibc-functions/sethostname.texi |   11 +-
 lib/sethostname.c|   71 ++
 m4/sethostname.m4|   21 ++
 modules/sethostname  |   32 +++
 4 files changed, 134 insertions(+), 1 deletions(-)
 create mode 100644 lib/sethostname.c
 create mode 100644 m4/sethostname.m4
 create mode 100644 modules/sethostname

diff --git a/doc/glibc-functions/sethostname.texi 
b/doc/glibc-functions/sethostname.texi
index 75cc8ca..7960eda 100644
--- a/doc/glibc-functions/sethostname.texi
+++ b/doc/glibc-functions/sethostname.texi
@@ -2,10 +2,15 @@
 @subsection @code{sethostname}
 @findex sethostname
 
-Gnulib module: ---
+Gnulib module: sethostname
 
 Portability problems fixed by Gnulib:
 @itemize
+@item
+On AIX 7.1, OSF/1 5.1 and Solaris 10 the declaration is missing.  On
+Minix 3.1.8, Cygwin, mingw, MSVC 9, Interix 3.5, BeOS the function is
+missing.  Provide a fallback for all platforms that returns -1 and
+sets ENOSYS.  Provide a specific implementation for Minix 3.1.8
 @end itemize
 
 Portability problems not fixed by Gnulib:
@@ -16,4 +21,8 @@ Minix 3.1.8, Cygwin, mingw, MSVC 9, Interix 3.5, BeOS.
 @item
 This function is not declared on some platforms:
 AIX 7.1, OSF/1 5.1, Solaris 10.
+@item
+On Solaris 10, the first argument is @code{char *} instead of
+@code{const char *} and the second parameter is @code{int} instead of
+@code{size_t}.
 @end itemize
diff --git a/lib/sethostname.c b/lib/sethostname.c
new file mode 100644
index 000..70d5955
--- /dev/null
+++ b/lib/sethostname.c
@@ -0,0 +1,71 @@
+/* sethostname emulation for glibc compliance.
+
+   Copyright (C) 2011 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see .  */
+
+/* Ben Walton  */
+
+#include 
+
+/* Unix API.  */
+
+/* Specification.  */
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+/* Set up to LEN chars of NAME as system hostname.
+   Return 0 if ok, -1 if error.  */
+
+int
+sethostname (const char *name, size_t len)
+{
+  /* glibc seems to allow a zero length hostname: bail on names that
+ are too long or too short */
+  if (len < 0 || len > HOST_NAME_MAX)
+{
+  errno = EINVAL;
+  return -1;
+}
+
+  /* NAME does not need to be null terminated so leave room to terminate
+ regardless of input. */
+  char hostname[len + 1];
+  strncpy(hostname, name, len);
+  hostname[len] = '\0';
+
+#ifdef __minix /* Minix */
+  FILE *hostf;
+
+  /* leave errno alone in this case as it will provide a more useful error
+ indication than overriding with ENOSYS */
+  if ((hostf = fopen("/etc/hostname.file", "w")) == NULL)
+return -1;
+  else
+{
+  fprintf(hostf, "%s\n", hostname);
+  fclose(hostf);
+  return 0;
+}
+#else
+  /* For platforms that we don't have a better option for, simply bail
+ out */
+  errno = ENOSYS;
+  return -1;
+#endif
+}
diff --git a/m4/sethostname.m4 b/m4/sethostname.m4
new file mode 100644
index 000..dbdbb39
--- /dev/null
+++ b/m4/sethostname.m4
@@ -0,0 +1,21 @@
+# sethostname.m4 serial 1
+dnl Copyright (C) 2011 Free Software Foundation, Inc.
+dnl This file is free software; the Free Software Foundation
+dnl gives unlimited permission to copy and/or distribute it,
+dnl with or without modifications, as long as this notice is preserved.
+
+# Ensure
+# - the sethostname() function,
+AC_DEFUN([gl_FUNC_SETHOSTNAME],
+[
+  AC_REQUIRE([gl_UNISTD_H_DEFAULTS])
+
+  AC_REPLACE_FUNCS([sethostname])
+  AC_CHECK_DECLS([sethostname])
+  if test $ac_cv_func_sethostname = no; then
+gl_PREREQ_HOST_NAME_MAX
+  fi
+  if test $ac_cv_have_decl_sethostname = no; then
+HAVE_DECL_SETHOSTNAME=0
+  fi
+])
diff --git a/modules/sethostname b/modules/sethostname
new file mode 100644
index 000..a0f36a2
--- /dev/null
+++ b/modules/sethostname
@@ -0,0 +1,32 @@
+Description:
+sethostname() function: Set machine's hostname.
+
+Files:
+lib/sethostname.c
+m4/sethostname.m4
+
+Depends-on:
+unistd
+gethostname
+errno   [test $HAVE_SETHOSTNAME = 0]
+
+configure.ac:
+gl_FUNC_SETHOSTNAME
+if te

[PATCH 3/3] Integrate the sethostname module into unistd

[Ben Walton]

Ensure that sethostname is accounted for within the unistd module.

Signed-off-by: Ben Walton 
---
 lib/unistd.in.h |   24 
 m4/unistd_h.m4  |7 +--
 modules/unistd  |3 +++
 3 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/lib/unistd.in.h b/lib/unistd.in.h
index f53f34b..b519019 100644
--- a/lib/unistd.in.h
+++ b/lib/unistd.in.h
@@ -683,6 +683,30 @@ _GL_WARN_ON_USE (getgroups, "getgroups is unportable - "
 # endif
 #endif
 
+#if @GNULIB_SETHOSTNAME@
+/* Set the host name of the machine.
+   The host name may or may not be fully qualified.
+
+   Put LEN bytes of NAME into the host name.
+   Return 0 if successful, otherwise, set errnon and return -1
+
+   Platforms with no ability to set the hostname return -1 and set
+   errno = ENOSYS. */
+#  if !@HAVE_SETHOSTNAME@ || !@HAVE_DECL_SETHOSTNAME@
+_GL_FUNCDECL_SYS (sethostname, int, (const char *name, size_t len)
+_GL_ARG_NONNULL ((1)));
+#  endif
+/* Need to cast, because on Solaris 10, the first parameter is not
+   const and the second parameter is int len.  */
+_GL_CXXALIAS_SYS_CAST (sethostname, int, (const char *name, size_t len));
+_GL_CXXALIASWARN (sethostname);
+#elif defined GNULIB_POSIXCHECK
+# undef gethostname
+# if HAVE_RAW_DECL_GETHOSTNAME
+_GL_WARN_ON_USE (sethostname, "sethostname is unportable - "
+ "use gnulib module sethostname for portability");
+# endif
+#endif
 
 #if @GNULIB_GETHOSTNAME@
 /* Return the standard host name of the machine.
diff --git a/m4/unistd_h.m4 b/m4/unistd_h.m4
index 57c8094..3ae47f9 100644
--- a/m4/unistd_h.m4
+++ b/m4/unistd_h.m4
@@ -43,8 +43,8 @@ AC_DEFUN([gl_UNISTD_H],
 fdatasync fsync ftruncate getcwd getdomainname getdtablesize getgroups
 gethostname getlogin getlogin_r getpagesize getusershell setusershell
 endusershell group_member lchown link linkat lseek pipe pipe2 pread pwrite
-readlink readlinkat rmdir sleep symlink symlinkat ttyname_r unlink unlinkat
-usleep])
+readlink readlinkat rmdir sethostname sleep symlink symlinkat ttyname_r
+unlink unlinkat usleep])
 ])
 
 AC_DEFUN([gl_UNISTD_MODULE_INDICATOR],
@@ -95,6 +95,7 @@ AC_DEFUN([gl_UNISTD_H_DEFAULTS],
   GNULIB_READLINKAT=0;   AC_SUBST([GNULIB_READLINKAT])
   GNULIB_RMDIR=0;AC_SUBST([GNULIB_RMDIR])
   GNULIB_SLEEP=0;AC_SUBST([GNULIB_SLEEP])
+  GNULIB_SETHOSTNAME=0;  AC_SUBST([GNULIB_SETHOSTNAME])
   GNULIB_SYMLINK=0;  AC_SUBST([GNULIB_SYMLINK])
   GNULIB_SYMLINKAT=0;AC_SUBST([GNULIB_SYMLINKAT])
   GNULIB_TTYNAME_R=0;AC_SUBST([GNULIB_TTYNAME_R])
@@ -132,6 +133,7 @@ AC_DEFUN([gl_UNISTD_H_DEFAULTS],
   HAVE_READLINK=1;AC_SUBST([HAVE_READLINK])
   HAVE_READLINKAT=1;  AC_SUBST([HAVE_READLINKAT])
   HAVE_SLEEP=1;   AC_SUBST([HAVE_SLEEP])
+  HAVE_SETHOSTNAME=1;AC_SUBST([HAVE_SETHOSTNAME])
   HAVE_SYMLINK=1; AC_SUBST([HAVE_SYMLINK])
   HAVE_SYMLINKAT=1;   AC_SUBST([HAVE_SYMLINKAT])
   HAVE_UNLINKAT=1;AC_SUBST([HAVE_UNLINKAT])
@@ -143,6 +145,7 @@ AC_DEFUN([gl_UNISTD_H_DEFAULTS],
   HAVE_DECL_GETLOGIN_R=1; AC_SUBST([HAVE_DECL_GETLOGIN_R])
   HAVE_DECL_GETPAGESIZE=1; AC_SUBST([HAVE_DECL_GETPAGESIZE])
   HAVE_DECL_GETUSERSHELL=1; AC_SUBST([HAVE_DECL_GETUSERSHELL])
+  HAVE_DECL_SETHOSTNAME=1; AC_SUBST([HAVE_DECL_SETHOSTNAME])
   HAVE_DECL_TTYNAME_R=1;  AC_SUBST([HAVE_DECL_TTYNAME_R])
   HAVE_OS_H=0;AC_SUBST([HAVE_OS_H])
   HAVE_SYS_PARAM_H=0; AC_SUBST([HAVE_SYS_PARAM_H])
diff --git a/modules/unistd b/modules/unistd
index 633ba31..f2125b3 100644
--- a/modules/unistd
+++ b/modules/unistd
@@ -66,6 +66,7 @@ unistd.h: unistd.in.h $(top_builddir)/config.status 
$(CXXDEFS_H) $(ARG_NONNULL_H
  -e 's/@''GNULIB_READLINK''@/$(GNULIB_READLINK)/g' \
  -e 's/@''GNULIB_READLINKAT''@/$(GNULIB_READLINKAT)/g' \
  -e 's/@''GNULIB_RMDIR''@/$(GNULIB_RMDIR)/g' \
+ -e 's/@''GNULIB_SETHOSTNAME''@/$(GNULIB_SETHOSTNAME)/g' \
  -e 's/@''GNULIB_SLEEP''@/$(GNULIB_SLEEP)/g' \
  -e 's/@''GNULIB_SYMLINK''@/$(GNULIB_SYMLINK)/g' \
  -e 's/@''GNULIB_SYMLINKAT''@/$(GNULIB_SYMLINKAT)/g' \
@@ -104,6 +105,7 @@ unistd.h: unistd.in.h $(top_builddir)/config.status 
$(CXXDEFS_H) $(ARG_NONNULL_H
  -e 's|@''HAVE_READLINK''@|$(HAVE_READLINK)|g' \
  -e 's|@''HAVE_READLINKAT''@|$(HAVE_READLINKAT)|g' \
  -e 's|@''HAVE_SLEEP''@|$(HAVE_SLEEP)|g' \
+ -e 's|@''HAVE_SETHOSTNAME''@|$(HAVE_SETHOSTNAME)|g' \
  -e 's|@''HAVE_SYMLINK''@|$(HAVE_SYMLINK)|g' \
  -e 's|@''HAVE_SYMLINKAT''@|$(HAVE_SYMLINKAT)|g' \
  -e 's|@''HAVE_UNLINKAT''@|$(HAVE_UNLINKAT)|g' \
@@ -115,6 +117,7 @@ unistd.h: unistd.in.h $(top_builddir)/config.status 
$(CXXDEFS_H) $(ARG_NONNULL_H
  -e 's|@''HAVE_DECL_GETLOGIN_R''@|$(HAVE_DECL_GETLOGIN_R)|g' \
  -e 's|@''HAVE_DECL_GETPAGESIZE''@|$(HAVE_DECL