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
