Re: [AD] Patches for allegro 4.1.9

[Philipp Thomas <pthomas@xxxxxxxxxx>]

* Eric Botcazou (ebotcazou@xxxxxxxxxx) [20030204 15:58]:

> What is your position on the autoconf 2.1x/autoconf 2.5x issue? Is it now 
> time to move to the latter (are you doing that at SuSE) ?

We've done so since autoconf 2.5x was out and you'll only find autoconf 2.53
on SuSE Linux 8.1 and up.
 
> I think we should go one step further and allow the user to override CFLAGS 
> at compile time. But this patch certainly can't hurt.

I wouldn't go that far, as the programmer probably knows best what
optimisation flags to use. And if users are knowledgable enough, they can
pass things like -march=athlon via XCFLAGS.

Hmm, now that I think about it, variables like XCFLAGS and XWFLAGS should
be declared via AC_ARG_VAR to make them precious. Quoting from autoconf.inf:

     Being precious means that
        - VARIABLE is 
C_SUBST''d.

        - VARIABLE is kept in the cache including if it was not
          specified on the ./configure' command line.  Indeed, while
          `Configure' can notice the definition of `CC' in ./configure
          CC=bizarre-cc', it is impossible to notice it in
          `CC=bizarre-cc ./configure', which, unfortunately, is what
          most users do.

I'll redo that patch to include the AC_ARG_VAR stuff.

> -	       if ((int)s - (int)get_filename(name) > 5)
> 
> I think you turned a safe signed comparison into a potentially unsafe 
> unsigned comparison (potentially only because s >= name in practice).
> So casting to 'long' seems to be safer here.

Casting it to long is also potentially unsafe, as the address of the pointer
might exceed the range of a signed long.

Why cast in the first place and not compare the pointers directly, .i.e.

   if ((s - get_filename(name)) > 5)

as the difference between pointers is always a signed integer?

> +			d->dp3 = (void *)((unsigned long long)d->dp3 + 1);
> 
> Why 'unsigned long long'? A typo?

Yepp, it's a typo. Thanks for catching that.
 
> I find that a bit radical: as soon as a comparison against sizeof() is 
> involved, we need to turn everything into size_t? I'd rather use an explicit 
> cast in the comparison.

You don't need to turn it into size_t, an explicit cast in the comparison is
also OK. But if, as in this case, the variable can never be less thn 0, why
not make it a size_t also and avoid the cast alltogether?

> --- src/linux/fbcon.c
> +++ src/linux/fbcon.c
> @@ -561,7 +561,7 @@
>  {
>     unsigned short r[256], g[256], b[256];
>     struct fb_cmap cmap;
> -   int i;
> +   __u32 i;
>  
>     cmap.start = from;
>     cmap.len = to-from+1;
> 
> Why do you need to use __u32 ?

I don't need to :) cmap.len is defined to be of type __u32 and i starts at
0, so any unsigned integer would suffice, but then why not use the same
types?

> >   - provides correct prototypes for declarations and definitions,
> >     mostly (void) instead of the incorrect ().
> 
> Why incorrect? The C99 grammar read:

I should have been more precise. It's not incorrect but the compiler can't
distinguish between K&R prototypes and an empty parameter list. So if you
use -Wstrict-prototypes (as I always do), you'll get a warning when you
don't use `void'.

> This change needs to be reflected in the docs.

Will you do it or should I send a patch?
 
> Why not simply main(void)?

I just missed to rechange that.

> Thanks for spotting these bugs.

You're wellcome :) I'd suggest to add -Wsign-compare to the warning flags to
catch such bugs in the future. Now that you've made a warning free compile
mandatory for checkins, such cases would have to be dealt with.

> +# But which you *need* for compiling on other platforms ...
> +Autoreqprov: on
> 
> Peter, could you comment on this problem ?

As I understand it, turning on Autoreqprov also gets you dependencies for
optional modules, which seem to be unwanted. But you have to accept those as
the explicit dependencies vary with the versions of libraries used for
compiling allegro, specially if the libraries (like glibc and pthreads) use
symbol versioning. And if you build allegro for a biarch linux platform
like x86-64, where both 32 *and* 64 bit libraries are available, allegro
*must* require the correct versions. For instance on x86-64 I get the
following dependencies for allegro:

ld-linux-x86-64.so.2()(64bit)  
libX11.so.6()(64bit)  
libXext.so.6()(64bit)  
libartsc.so.0()(64bit)  
libasound.so.2()(64bit)  
libaudiofile.so.0()(64bit)  
libc.so.6()(64bit)  
libdl.so.2()(64bit)  
libesd.so.0()(64bit)  
libm.so.6()(64bit)  
libpthread.so.0()(64bit)  
libc.so.6(GLIBC_2.2.5)(64bit)  
libc.so.6(GLIBC_2.3)(64bit)  
libdl.so.2(GLIBC_2.2.5)(64bit)  
libm.so.6(GLIBC_2.2.5)(64bit)  
libpthread.so.0(GLIBC_2.2.5)(64bit)  
libpthread.so.0(GLIBC_2.3.2)(64bit)  

> P.S: Do you want the name of your employer to be explicitly mentioned in the 
> Changes and Thanks files (it will already be visible in your mangled email 
> address)?

Explicit mentioning isn't necessary as email address suffices.

Philipp

-- 
Philipp Thomas <pthomas@xxxxxxxxxx>
Development, SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nuremberg, Germany