Re: warn-on-use preview

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Bruno Haible on 1/1/2010 7:11 AM:
> 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.

That, and the whole point of poisoning variables is to inform the
developer that they are doing something non-portable in the first place.
It is the same as with using #undef to poison a macro - the compiler
behavior is not as clean as with the warning attribute (a failed
compilation, rather than a nice message and compilation resuming if
- -Werror was not specified), but the end result is the same: because of the
compilation failure, the developer knows to fix something.  And the fix in
this case is to import the environ module.

> 
> 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;
>    ])])

Actually, it looks more like:

AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
#if HAVE_LOCALE_H
# include <locale.h>
#endif
#if HAVE_LIBINTL_H
# include <libintl.h>
#endif
]], [[#undef setlocale
(void) setlocale;]])])

but that's details.

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

OK, I'll work on refactoring that to emit one AC_COMPILE_IFELSE loop per
header.  By the way, that will fail to poison identifiers that are
declared in the wrong header, but hopefully there aren't too many of those
(especially now that our unit tests can detect that, and we are pushing
fixes into platforms like cygwin as fast as we find those bugs).

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

Okay, I'll make stdio.in.h a bit longer (at which point the commit log can
be shorter).

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

Good point.  And particularly true if our rpl_fseeko might need to disable
the warning as part of calling fseek under the hood for mingw (hmm, that's
one test I haven't run yet - using only the fseeko module on mingw).

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

Sure.

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

Good point.

> 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)                                        \

Sounds reasonable.  But I'm glad you liked my approach at poisoning those
identifiers.  By the way, is there any reason we don't have an fpclassify
module yet?

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAks+BaoACgkQ84KuGfSFAYBb7QCbBJN+zZTWq789LZC9w1Q1qSc7
c0oAoLkuVX6taCTgIYAEHOxiriR6Mo1m
=fa0j
-----END PGP SIGNATURE-----