GNOME Bugzilla – Bug 342390
Code fixes for -ansi -pedantic build
Last modified: 2006-06-16 16:24:49 UTC
Please describe the problem: CVS build fails gratuitously with -ansi -pedantic. The attached patch fixes (most) of the issues: * Added _POSIX_C_SOURCE and _BSD_SOURCE feature test macros to configure.in. (f.ex. gimpsignal.c needs the POSIX bit to get "struct sigaction", and tile-swap.c needs BSD to get S_IREAD and S_IWRITE) * Added call to AC_C_INLINE, as you can't assume that the "inline" keyword is available. (Is available as __inline in GCC's ANSI mode) * plug-ins/common/tiff.c uses va_copy(), which is C99. GCC makes this available in non-C99 mode as __va_copy(), but there was no fallback mechanism to use this. Added checks for va_copy() and __va_copy() to configure.in, and some preprocessor magic to tiff.c to use whichever definition is available (whether implemented as a function or as a macro). You might want to move these conditionals into a common header file, so that other source files can make use of them... * Fixed a couple instances of C++-style comments. * Fixed a problem in plug-ins/Lighting/ where some GtkWidget* variable declarations were given in a header file (without the "extern" keyword), causing the linker to choke on multiply-defined symbols. (The same problem exists in plug-ins/{gfig,jpeg,metadata}/, but those have a much larger number of variables declared in their headers, and it may be better for someone closer to the project to fix that code.) Steps to reproduce: My CFLAGS are "-O0 -g3 -fno-common -ansi -pedantic" plus a boatload of -Wxxx flags, on GCC 4.0. Give 'em a try. Actual results: Expected results: Does this happen every time? Other information:
Created attachment 65880 [details] [review] The aforementioned patch The insertions into configure.in are located a bit arbitrarily; you might want to add those lines at different points in the file.
One further issue that I failed to catch: libgimpbase/gimpsignal.c is missing an #include"config.h" directive. (That file, in particular, really needs it.)
Thanks for noticing these. However, not all of your changes are correct: > * Added _POSIX_C_SOURCE and _BSD_SOURCE feature test macros to configure.in. > (f.ex. gimpsignal.c needs the POSIX bit to get "struct sigaction", and > tile-swap.c needs BSD to get S_IREAD and S_IWRITE) The policy is to put these in the files themselves, and not in the global config.h header. Also, tile-swap.c can simply be changed to use S_IRUSR and S_IWUSR. > * Added call to AC_C_INLINE, as you can't assume that the "inline" keyword is > available. (Is available as __inline in GCC's ANSI mode) We can assume it is available, since glib hides the details from us. > * plug-ins/common/tiff.c uses va_copy(), which is C99. GCC makes this available > in non-C99 mode as __va_copy(), but there was no fallback mechanism to use > this. Added checks for va_copy() and __va_copy() to configure.in, and some > preprocessor magic to tiff.c to use whichever definition is available (whether > implemented as a function or as a macro). You might want to move these > conditionals into a common header file, so that other source files can make use > of them... Well, we use G_VA_COPY, not va_copy directly. You want to file a bug against glib, preferably with a patch implements a fallback to __va_copy while under __STRICT_ANSI__. I'll commit the good parts of your patch.
2006-05-20 Manish Singh <yosh@gimp.org> Fixes to address -ansi -pedantic compilation (bug #342390). Thanks goes to Daniel Richard G. for noticing and suggesting fixes. * libgimpbase/gimpsignal.c: #include "config.h" and define __POSIX_SOURCE for sigaction stuff. * app/base/tile-swap.c (tile_swap_test): use more portable S_IRUSR and S_IWUSR, instead of S_IREAD and S_IWRITE. * plug-ins/common/ripple.c * plug-ins/imagemap/imap_main.c: use C89 comments. * plug-ins/Lighting/lighting_preview.h: don't define spin widget variables here... * plug-ins/Lighting/lighting_ui.[ch]: ... and instead take care of them here.
Created attachment 65911 [details] [review] Follow-on patch (I think you meant _POSIX_SOURCE, not __POSIX_SOURCE :-) For my part, I forgot that glib does provide a lot of workarounds for this) New patch, which places the feature test macros in the files that need them. Also, gimpsignal.c needed a newer POSIX rev. AC_C_INLINE was needed for modules/cdisplay_{lcms,proof}.c, which include lcms.h (which makes use of "inline") before any glib/gtk headers. Worked around the error by including <glib.h> first. Fixed the metadata plug-in, as the problem was trivial. Please try building with -fno-common on your end... the gfig and jpeg plug-ins still have the same multiple-definition problem as Lighting and metadata did. Will have a closer look at glib's definition of G_VA_COPY().
Please file separate bugs for -fno-common issues, one per plug-in.
Could you provide comments with the #define _FOO_SOURCE bits detailing why they are needed?
Created attachment 65921 [details] [review] Improved patch Comments added, and some minor POSIX vs. BSD vs. SVID confusion cleared up. Also improved the handling of PATH_MAX in gqbist.c; in particular, don't use _MAX_PATH (which appears to be more of a DOS/Win32 thing) unless it is #defined, to avoid confusion when it doesn't exist.
The patch looks good to me. Yosh, what do you think?
Yep, looks good. Thanks! 2006-05-29 Manish Singh <yosh@gimp.org> * app/errors.c * app/main.c * app/file/gimprecentlist.c * libgimp/gimp.c * libgimpbase/gimpsignal.c * modules/cdisplay_lcms.c * modules/cdisplay_proof.c * modules/controller_midi.c * plug-ins/common/gqbist.c * plug-ins/metadata/xmp-schemas.h: miscellaneous fixes for building with -ansi -pedantic. Mostly #define _FOO_SOURCE stuff. Thanks to Daniel Richard G. for the patch. Fixes bug #342390.
After this change I get the following compiler warning in app/file: gimprecentlist.c: In function ‘gimp_recent_list_write_raw’: gimprecentlist.c:379: warning: implicit declaration of function ‘ftruncate’ gimprecentlist.c:392: warning: implicit declaration of function ‘fsync’
2006-05-30 Manish Singh <yosh@gimp.org> * app/file/gimprecentlist.c: #define _GNU_SOURCE instead of _SVID_SOURCE, so we get all the declarations we need. Fixes bug #342390.
Have to be careful there; non-GNU POSIX systems won't recognize _GNU_SOURCE. (This is why I stuck with the more specific feature test macros.) An ANSI build turns up a _lot_ of files missing function prototypes, e.g. for fdopen(), rint() and g_date_set_time(). Kind of a shame that the _FOO_SOURCE stuff can't go in config.h... :-)
Please see bug 344203. Daniel, perhaps you can join the discussion on this report?
As per bug 344203, we've switched to using _GNU_SOURCE. In fact, the glibc docs explicitly say these macros are *not* to be used for verifying that a program conforms to these standards, but rather, to exclude extensions that result in namespace conflicts.