Re: warn-on-use preview

[Bruno Haible]

Hi Eric,

Eric Blake wrote:
> I won't push this until it's had a bit longer for reviews.

Yes, for reviewing 4 patches like this you need to give me a day at least.

>       [1/4] warn-on-use: new module

The recommendation how to deal with variables like 'environ':

  static inline char ***rpl_environ (void) { return &environ; }
  # define environ (*rpl_environ ())

will not work when a program does

  static char*** foo = &environ;

but this is a rare case that will probably not occur. So that's fine.

The implementation of gl_WARN_ON_USE_PREPARE looks strange and brittle to me,
when some header is implied that interferes with another header. Say, I write

   gl_WARN_ON_USE_PREPARE([locale.h], [setlocale])
   gl_WARN_ON_USE_PREPARE([libintl.h], [gettext])

then it will check for setlocale using

   AC_COMPILE_IFELSE([AC_LANG_PROGRAM([
     #include <locale.h>
     #include <libintl.h>
     #undef setlocale
     (void) setlocale;
   ])])

which may or may not be the same as the intended

   AC_COMPILE_IFELSE([AC_LANG_PROGRAM([
     #include <locale.h>
     #undef setlocale
     (void) setlocale;
   ])])

I would prefer an implementation where the expansion of
  gl_WARN_ON_USE_PREPARE([locale.h], [setlocale])
does not depend on other invocations of gl_WARN_ON_USE_PREPARE, even if it
leads to a larger 'configure' file.

>       [2/4] stdio: warn on suspicious uses
> ... Also, rewrite the ftell/ftello warnings.  I almost feel 
> bad that the commit log is about as long as the patch.

Yes, some of the commit log would make sense as a comment in stdio.in.h.

> +   In other words, _GL_NO_LARGE_FILES is a witness macro that states
> +   that arbitrarily limiting to 32-bit offsets is safe for a given
> +   program

Here I would say "... safe for a given compilation unit". The
_GL_NO_LARGE_FILES macro is often applied to only some compilation units
among a program.

>  AC_DEFUN([gl_STDIO_H],
>  [
> +  AC_REQUIRE([AC_C_INLINE])
>    AC_REQUIRE([gl_STDIO_H_DEFAULTS])
>    gl_CHECK_NEXT_HEADERS([stdio.h])

Could you insert the AC_C_INLINE line one line below? The common idiom wants
that AC_REQUIRE([gl_STDIO_H_DEFAULTS]) is the very first thing inside
gl_STDIO_H.

> --- a/tests/test-freadable.c
> +++ b/tests/test-freadable.c
> @@ -20,16 +20,13 @@
> 
>  #include "freadable.h"
> 
> +/* None of the files accessed by this test are large, so disable the
> +   fseek link warning if we are not using the gnulib fseek module.  */
> +#define _GL_NO_LARGE_FILES

These added lines should come between #include <config.h> and
#include "freadable.h", because the latter may include <stdio.h>, and
setting _GL_NO_LARGE_FILES after you have already included <stdio.h> has
no effect.

Likewise in tests/test-freading.c, tests/test-fwritable.c, 
tests/test-fwriting.c,
tests/test-getopt.c. 

>       [3/4] unistd: warn on use of environ without module

Looks fine.

>       [4/4] math: add portability warnings for classification macros

> +#define _GL_WARN_REAL_FLOATING_DECL(func) \
> +static inline int                                                   \
> +rpl_ ## func (float f, double d, long double l, int which)          \
> +{                                                                   \
> +  switch (which)                                                    \
> +    {                                                               \
> +    case 1: return func (f);                                        \
> +    case 2: return func (d);                                        \
> +    default: return func (l);                                       \
> +    }                                                               \
> +}                                                                   \
> +_GL_WARN_ON_USE (rpl_ ## func, #func " is unportable - "            \
> +                 "use gnulib module " #func " for portability")
> +#define _GL_WARN_REAL_FLOATING_IMPL(func, value) \
> +  ({                                                                \
> +    __typeof__ (value) __x = (value);                               \
> +    rpl_ ## func (__x, __x, __x,                                    \
> +                  (sizeof __x == sizeof (float) ? 1                 \
> +                   : sizeof __x == sizeof (double) ? 2 : 3));       \
> +  })

There is some risk that this leads to link errors or warnings on some
systems where 'long double' is not correctly supported. For example, on HP-UX,
the isinf or some similar macro refers to an undefined symbol when used on a
'long double'. And on MacOS X 10.3.x, any use of 'long double' leads to a
warning.

Therefore I think it is safer to write this using 3 separate inline functions
for 'float', 'double', and 'long double':

#define _GL_WARN_REAL_FLOATING_DECL(func) \
static inline int                                                   \
rpl_ ## func ## _f (float x)                                        \
{                                                                   \
  return func (x);                                                  \
}                                                                   \
static inline int                                                   \
rpl_ ## func ## _d (double x)                                       \
{                                                                   \
  return func (x);                                                  \
}                                                                   \
static inline int                                                   \
rpl_ ## func ## _l (long double x)                                  \
{                                                                   \
  return func (x);                                                  \
}                                                                   \
_GL_WARN_ON_USE (rpl_ ## func ## _f, #func " is unportable - "      \
                 "use gnulib module " #func " for portability")     \
_GL_WARN_ON_USE (rpl_ ## func ## _d, #func " is unportable - "      \
                 "use gnulib module " #func " for portability")     \
_GL_WARN_ON_USE (rpl_ ## func ## _l, #func " is unportable - "      \
                 "use gnulib module " #func " for portability")     \
#define _GL_WARN_REAL_FLOATING_IMPL(func, value) \
  ({                                                                \
    __typeof__ (value) __x = (value);                               \
    (sizeof __x == sizeof (float) ? rpl_ ## func ## _f (__x) :      \
     sizeof __x == sizeof (double) ? rpl_ ## func ## _d (__x) :     \
     rpl_ ## func ## _l (__x));                                     \
  })

Bruno