[Gegl-developer] babl portability patches, and a test failure

[Sven Neumann]

Hi,

On Tue, 2009-02-17 at 14:12 +0000, Gary V. Vaughan wrote: 
> It is very difficult to compile babl-0.0.22 unless you are
> using gcc and gmake with the GNU coreutils and ruby, on a
> linux host.
> 
> With the attached patches I was able to successfully build
> everything with the vendor compilers, and pass all but one
> test (failure output below) on these 30 architectures:
> 
>   SuSE SLES 10 (x86_64 and i686);
>   Redhat RHEL3, RHEL4 and RHEL5 (x86_64 and i686);
>   Redhat 7.1, 9 and RHEL 2.1 (i686);
>   Solaris 10 (x86 and sparc);
>   Solaris 2.6, 7, 8 and 9 (sparc);
>   AIX 4.3.3, 5.1, 5.2, 5.3 and 6.1 (powerpc);
>   HPUX 11.31 (ia64);
>   HPUX 11.23 (pa-risc and ia64);
>   HPUX 10.20, 11.0 and 11.11 (pa-risc);
>   IRIX 6.5 (mips);
>   Tru64 Unix 5.1 (ev5).

Nice work. But unfortunately that i a huge pile of changes in more or
less one large patch. You don't happen to be able to provide this as a
changeset broken up into smaller changes? Some of the changes are
definitely good, but we might want to handle a few things differently.

> * build the extensions with libtool

I think that is IMO a good idea even though it will increase build time
significantly.

> * variadic macros are not portable

As a result you made the babl_log() macro a lot more difficult to use.
IMO we should rather add a test for variadic macros to the configure
script (this can be copied from glib's configure.in) and then define
babl_log() similar to what GLib does in gmessages.h:

#ifdef G_HAVE_ISO_VARARGS
#define g_message(...)  g_log (G_LOG_DOMAIN,         \
                               G_LOG_LEVEL_MESSAGE,  \
                               __VA_ARGS__)
#else if defined(G_HAVE_GNUC_VARARGS)
#define g_message(format...)    g_log (G_LOG_DOMAIN,         \
                                       G_LOG_LEVEL_MESSAGE,  \
                                       format)
#else
g_message (const gchar *format,
           ...)
{
  va_list args;
  va_start (args, format);
  g_logv (G_LOG_DOMAIN, G_LOG_LEVEL_MESSAGE, format, args);
  va_end (args);
}
#endif


>  * don't use C++ comments

That is OK. But I saw that your patch removes comments in some places.
That is not OK, they should be converted to /* ... */ comments instead.

>  * for gnulib and autoconf to work properly, every .c file
>    needs to #include <config.h> before anything else!

Yes, definitely!


Sven



Sven