Re: [AD] Patches for allegro 4.1.9

[Eric Botcazou <ebotcazou@xxxxxxxxxx>]

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

Ok. Peter already proposed the move some time ago, but I wasn't too 
enthusiastic. We will go for it on the dev branch (4.1.x series) then.

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

Note that you can already specify -mcpu and -march options at configure time 
(see --enable-opts and --enable-exclopts).

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

Thanks.

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

Ok, I'll make the correction.

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

The function is a method of the INTERNAL_MOUSE_DRIVER class, so we would need 
to modify every mouse driver.

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

I hate double-underscored types :-)

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

Ok.

> > This change needs to be reflected in the docs.
>
> Will you do it or should I send a patch?

I'll do it.

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

I'm going be more drastic: Allegro has compiled for years with -Wall -W 
-Werror under DOS and Windows so I don't see why the Unix port should be 
less strict. Your patch is almost sufficient to achieve this goal (there are 
some incomplete initializers that I'm going to fill).

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

Thanks for the explanation. I'll turn it on for the 4.1.x series.

-- 
Eric Botcazou