| Re: [AD] Patches for allegro 4.1.9 |
[ Thread Index |
Date Index
| More lists.liballeg.org/allegro-developers Archives
]
> Now that my allegro package builds correctly, here are the diffs I came
> up. These are all against 4.1.9.
Thank you very much for your contribution!
> The first one (allegro-autoconf.dif) contains the autoconf stuff:
>
> - configure.in now requires autoconf 2.53 or higher.
> - The use of AH_TEMPLATE and the third argument of AC_DEFINE*
> makes acconfig.h obsolete.
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) ?
> - configure adds XCFLAGS and XWFLAGS to CFLAGS if defined. This
> makes it possible to pass in compiler flags that either override
> what configure.in sets or adds to them. This way a packager like
> me can easily incorporate RPM_OPT_FLAGS and add warning flags that
> would otherwise not be set.
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'd recommend to rename aclocal.m4 to acinclude.m4 to not get aclocal.m4
> overwritten when user calls aclocal. And, of cause, acconfig.h can be
> deleted after applying the diff :)
Ok, I'll do it.
> The second diff (allegro-cleanup.dif) does three things:
>
> - use 'unsigned long' when doing pointer<->integer casts.
I may have been a little intrepid when I told you that 'unsigned long'
(instead of 'long int') was the preferred way to deal with pointer<->integer
conversions. I think the correct way is actually a mix of both.
--- docs/src/makedoc/makehtml.c
+++ docs/src/makedoc/makehtml.c
@@ -535,7 +535,7 @@
else {
strcpy(name, filename);
s = get_extension(name)-1;
- if ((int)s - (int)get_filename(name) > 5)
+ if ((unsigned long)s - (unsigned long)get_filename(name) > 5)
s = get_filename(name)+5;
sprintf(s, "%03d.%s", section_number, html_extension);
_hfprintf("<li><a href=\"%s\">%s</a>\n", get_filename(name),
ALT_TEXT(toc));
@@ -703,7 +703,7 @@
_close_html_file(_file);
strcpy(buf, filename);
s = get_extension(buf)-1;
- if ((int)s - (int)get_filename(buf) > 5)
+ if ((unsigned long)s - (unsigned long)get_filename(buf) > 5)
s = get_filename(buf)+5;
sprintf(s, "%03d.%s", section_number-1, html_extension);
printf("writing %s\n", buf);
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.
--- src/guiproc.c
+++ src/guiproc.c
@@ -1303,17 +1303,17 @@
thisitem = (*(getfuncptr)d->dp)(i, NULL);
failure = FALSE;
- if ((int)d->dp3 < ustrlen(thisitem)) {
- for (a=0; a<(int)d->dp3; a++) {
+ if ((int)((unsigned long)d->dp3) < ustrlen(thisitem)) {
+ for (a=0; a < (int)((unsigned long)d->dp3); a++) {
if (utolower(ugetat(thisitem, a)) != utolower(ugetat(selected, a))) {
failure = TRUE;
break;
}
}
- if ((!failure) && (utolower(ugetat(thisitem, (int)d->dp3)) ==
utolower(c))) {
+ if ((!failure) && (utolower(ugetat(thisitem, (int)(long int)d->dp3))
== utolower(c))) {
d->d1 = i;
- d->dp3 = (void *)((int)d->dp3 + 1);
+ d->dp3 = (void *)((unsigned long long)d->dp3 + 1);
if (sel) {
for (i=0; i<listsize; i++)
Why 'unsigned long long'? A typo?
> - fixes those cases where signed and unsigned where mixed
> in comparisons and conditional expressions.
--- setup/setup.c
+++ setup/setup.c
@@ -1327,7 +1327,7 @@
char buffer[256];
char tmp1[256], tmp2[256], tmp3[64], tmp4[64];
AL_CONST char *drv_name;
- int count;
+ size_t count;
_DRIVER_INFO *list;
int list_size;
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.
--- src/linux/fbcon.c
+++ src/linux/fbcon.c
@@ -159,7 +159,7 @@
my_mode.xres = w;
my_mode.yres = h;
my_mode.xres_virtual = MAX(w, v_w);
- my_mode.yres_virtual = MAX(MAX(h, v_h), fix_info.smem_len /
(my_mode.xres_virtual * BYTES_PER_PIXEL(color_depth)));
+ my_mode.yres_virtual = MAX(MAX(h, v_h), (int)(fix_info.smem_len /
(my_mode.xres_virtual * BYTES_PER_PIXEL(color_depth))));
break;
case 1:
@@ -172,7 +172,7 @@
case 2:
/* see if we can fake a smaller mode (better than nothing) */
- if ((my_mode.xres < w) || (my_mode.yres < h) || (v_w > w) || (v_h > h))
+ if ((my_mode.xres < (__u32)w) || (my_mode.yres < (__u32)h) || (v_w > w)
|| (v_h > h))
continue;
fb_approx = TRUE;
break;
@@ -498,9 +498,9 @@
/* fb_vsync:
* Waits for a retrace.
*/
-static void fb_vsync()
+static void fb_vsync(void)
{
- int prev;
+ __u32 prev;
#ifdef FBIOGET_VBLANK
@@ -548,7 +548,7 @@
prev = retrace_count;
do {
- } while (retrace_count == prev);
+ } while ((__u32)retrace_count == prev);
}
}
@@ -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 ?
> - provides correct prototypes for declarations and definitions,
> mostly (void) instead of the incorrect ().
Why incorrect? The C99 grammar read:
declarator:
pointer-opt direct-declarator
direct-declarator:
identifier
( declarator )
direct-declarator [ assignment-expression-opt ]
direct-declarator [ * ]
direct-declarator ( parameter-type-list )
direct-declarator ( identifier-list-opt )
And:
[#10] An identifier list declares only the identifiers of
the parameters of the function. An empty list in a function
declarator that is part of a function definition specifies
that the function has no parameters. The empty list in a
function declarator that is not part of a function
definition specifies that no information about the number or
types of the parameters is supplied.
But I'll apply the modifications anyway.
--- include/allegro/gui.h
+++ include/allegro/gui.h
@@ -77,7 +77,7 @@
int size; /* number of items in the menu */
int sel; /* selected item */
int x, y, w, h; /* screen position of the menu */
- int (*proc)(); /* callback function */
+ int (*proc)(void); /* callback function */
BITMAP *saved; /* saved what was underneath it */
int mouse_button_was_pressed; /* set if mouse button pressed on last
iteration */
This change needs to be reflected in the docs.
--- tests/afinfo.c
+++ tests/afinfo.c
@@ -779,7 +779,7 @@
#else /* ifdef VBE/AF cool on this platform */
-int main()
+int main(int argc, char **argv)
{
allegro_init();
allegro_message("Sorry, the AFINFO program only works on DOS or
Linux\n");
Why not simply main(void)?
> I'd like to point out one of the fixes, made at a few places,
>
> --- src/linux/lconsole.c
> +++ src/linux/lconsole.c
> @@ -243,9 +243,13 @@
>
> do {
> ret = write(STDERR_FILENO, msg, strlen(msg));
> - if ((ret < 0) && (errno != EINTR))
> - break;
> - } while (ret < strlen(msg));
> + if (ret < 0) {
> + if (errno != EINTR)
> + break;
> + else
> + ret = 0;
> + }
> + } while ((size_t)ret < strlen(msg));
>
> Just think about what that comparison in while() yields when the
> comparison is done unsigned and ret is negative ...
Oh! yes, this defeats the purpose of the (errno != EINTR) test. Thanks for
spotting these bugs.
> The third patch (allegro-spec.diff) is for your convenience only, as our
> specfile is different. The changes I made:
>
> - Switch on Autorecprov. This is mandatory to get correct packages on
> systems with other versions of the libraries (newer, 64bit vs. 32bit
> etc.). Despite getting dependencies for modules, it can't be avoided
> untill a version of rpm comes up that offers 'nice to have but not
> necessary' dependencies.
> - Provides german summaries and descriptions.
> - Uses DESTDIR to install.
> - Makes allegro-devel require the matching version of its base package.
I have to say that I'm not familiar with RPM spec files at all. The changes
looks ok, except the first one:
@@ -19,13 +20,9 @@
#BuildRequires: texinfo
# Automatic dependency generation picks up module dependencies
# which is exactly what we don't want...
-Autoreq: off
+# But which you *need* for compiling on other platforms ...
+Autoreqprov: on
Peter, could you comment on this problem ?
> Please give me feedback if I botched somewhere.
No need to post modified patches, I'll do the corrections (if any) myself
once we have settled on the fixes for the issues I'm raising here.
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)?
--
Eric Botcazou