[PATCH 1/2] build: accommodate gnulib's new warnings with --enable-gcc-warnings

[Jim Meyering]

FYI, more warning-avoidance and a gnulib update:

From 337e3ad58a7b6be7f146b2c14065b5188e329943 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@...>
Date: Tue, 29 Nov 2011 18:33:31 +0100
Subject: [PATCH 1/2] build: accommodate gnulib's new warnings with
 --enable-gcc-warnings

* configure.ac (WERROR_CFLAGS): Disable two new warnings:
-Wno-format-nonliteral, -Wno-unsuffixed-float-constants.
* gzip.h (bi_reverse): Declare with _GL_ATTRIBUTE_CONST.
(gzip_base_name): Declare with _GL_ATTRIBUTE_PURE.
---
 configure.ac |    2 ++
 gzip.h       |    4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 4013dfc..6e3e19b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -103,6 +103,8 @@ if test "$gl_gcc_warnings" = yes; then
   gl_WARN_ADD([-Wno-unused-parameter]) # Too many warnings for now
   gl_WARN_ADD([-Wno-overflow])         # util.c
   gl_WARN_ADD([-Wno-type-limits])      # util.c
+  gl_WARN_ADD([-Wno-format-nonliteral])
+  gl_WARN_ADD([-Wno-unsuffixed-float-constants])

   # In spite of excluding -Wlogical-op above, it is enabled, as of
   # gcc 4.5.0 20090517, and it provokes warnings in cat.c, dd.c, truncate.c
diff --git a/gzip.h b/gzip.h
index 568ffb0..49a9dbd 100644
--- a/gzip.h
+++ b/gzip.h
@@ -292,7 +292,7 @@ extern off_t flush_block (char *buf, ulg stored_len, int eof);
         /* in bits.c */
 extern void     bi_init    (file_t zipfile);
 extern void     send_bits  (int value, int length);
-extern unsigned bi_reverse (unsigned value, int length);
+extern unsigned bi_reverse (unsigned value, int length) _GL_ATTRIBUTE_CONST;
 extern void     bi_windup  (void);
 extern void     copy_block (char *buf, unsigned len, int header);
 extern int     (*read_buf) (char *buf, unsigned size);
@@ -307,7 +307,7 @@ extern void flush_window  (void);
 extern void write_buf     (int fd, voidp buf, unsigned cnt);
 extern int read_buffer    (int fd, voidp buf, unsigned int cnt);
 extern char *strlwr       (char *s);
-extern char *gzip_base_name (char *fname);
+extern char *gzip_base_name (char *fname) _GL_ATTRIBUTE_PURE;
 extern int xunlink        (char *fname);
 extern void make_simple_name (char *name);
 extern char *add_envopt   (int *argcp, char ***argvp, char const *env);
--
1.7.8.rc4


From d0aa88a5658ae560b3cf3c561d29a1ea1b7aa679 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@...>
Date: Tue, 29 Nov 2011 18:18:55 +0100
Subject: [PATCH 2/2] build: update gnulib submodule to latest

---
 gnulib |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gnulib b/gnulib
index 667561d..908690c 160000
--- a/gnulib
+++ b/gnulib
@@ -1 +1 @@
-Subproject commit 667561d720dd1e22b51fe5d308a8f94d49d46b85
+Subproject commit 908690cb743e69c73b42ae310807b29800c8764b
--
1.7.8.rc4

[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.

[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?

[eblake]

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   eblake@...    [phone]
Libvirt virtualization library http://libvirt.org

[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)
 #if HAVE_WORKING_FORK
   struct tempnode *saved_temphead;
   int saved_errno;
-  double wait_retry = 0.25;
+  double wait_retry = 0.25D;
   pid_t pid IF_LINT ( = -1);
   struct cs_status cs;

diff --git a/src/stat.c b/src/stat.c
index 4e5dbce..b20da0b 100644
--- a/src/stat.c
+++ b/src/stat.c
@@ -544,7 +544,7 @@ static int
 out_minus_zero (char *pformat, size_t prefix_len)
 {
   make_format (pformat, prefix_len, "'-+ 0", ".0f");
-  return printf (pformat, -0.25);
+  return printf (pformat, -0.25D);
 }

 /* Output the number of seconds since the Epoch, using a format that
diff --git a/src/tail.c b/src/tail.c
index 1641a12..ade0b36 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -2090,7 +2090,7 @@ main (int argc, char **argv)
   /* The number of seconds to sleep between iterations.
      During one iteration, every file name or descriptor is checked to
      see if it has changed.  */
-  double sleep_interval = 1.0;
+  double sleep_interval = 1.0D;

   initialize_main (&argc, &argv);
   set_program_name (argv[0]);

[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.

[eblake]

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   eblake@...    [phone]
Libvirt virtualization library http://libvirt.org

[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.

[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.

[eblake]

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 <eblake@...>
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 <eblake@...>
---
 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  <eblake@...>

+ 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   eblake@...    [phone]
Libvirt virtualization library http://libvirt.org

[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.

[eblake]

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   eblake@...    [phone]
Libvirt virtualization library http://libvirt.org

[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.

[Simon Josefsson-2]

Paul Eggert <eggert@...> writes:

> 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.

I'm opposed to this -- the philosophy behind the manywarnings module is
to enable all possible kind of warnings that you can get from gcc and
then each project has to disable the warnings that they decide are not
worth fixing.  That decision has to be different for each project.  I
believe this is fairly well described in doc/manywarnings.texi.

I'm not opposed to create a somewarnings.m4 or similar that filters out
some warnings, if you would find that useful.

FWIW, I have not had to disable the above flags in any of my projects,
and I would want to be informed if I introduce new code that triggers
them.

/Simon

[Simon Josefsson-2]

Eric Blake <eblake@...> writes:

> 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:

Please revert or change this -- like other warnings, this warning is
useful in some projects and not useful in others, and the idea with
manywarnings.m4 is that all warnings should be enabled and each project
has to disable the warnings they don't like incrementally.  See
doc/manywarnings.texi for background.

The function you added modified was this one:

# gl_MANYWARN_ALL_GCC(VARIABLE)
# -----------------------------
# Add all documented GCC (currently as per version 4.4) warning
# parameters to variable VARIABLE.

I'm fine with creating another function gl_MANYWARN_ALL_REASONEBLE_GCC
or similar if you prefer that, but let's keep the base function clean
from subjective filtering of warning flags.  It should just generate a
list of all warning flags that can possibly be enabled in gcc.

/Simon