Re: [AD] Patches for allegro 4.1.9 |
[ Thread Index |
Date Index
| More lists.liballeg.org/allegro-developers Archives
]
> > 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