dk / Prima

prima.eu.org
Other
106 stars 27 forks source link

Harmonize Prima Bool type with X11 Bool macro #106

Closed ppisar closed 5 months ago

ppisar commented 5 months ago

After upgrading GCC to version 14 Prima fails to compile like this on 32-bit x86 platform with perl configured with -Duse64bitint:

img/codec_Xpm.c: In function ‘apc_img_codec_Xpm’: img/codec_Xpm.c:635:28: error: assignment to ‘Bool ()(struct ImgCodec , struct _ImgLoadFileInstance )’ {aka ‘long int ()(struct ImgCodec , struct _ImgLoadFileInstance )’} from incompatible pointer type ‘int ()(struct ImgCodec , struct _ImgLoadFileInstance )’ [-Wincompatible-pointer-types] 635 | vmt. load = load; | ^ img/codec_Xpm.c:639:28: error: assignment to ‘Bool ()(struct ImgCodec , struct _ImgSaveFileInstance )’ {aka ‘long int ()(struct ImgCodec , struct _ImgSaveFileInstance )’} from incompatible pointer type ‘int ()(struct ImgCodec , struct _ImgSaveFileInstance )’ [-Wincompatible-pointer-types] 639 | vmt. save = save; | ^

The cause is that Bool in a type on the left side of the assignment is declared before including <X11/Xpm.h>. At that time Prima defines a Bool as Perl's I32 (which is long int on 32-bit -Duse64bitint; see "include/apricote.h" and Perl ), but the right side is declared after <X11/Xpm.h> when Bool is a macro expanding to int (see <X11/Xdefs.h>).

This patch changes Prima's Bool to int to match what X11 uses.

CPAN RT#151523

dk commented 5 months ago

I'd say that its Xpm.h in the wrongs here, not apricot.h. Xpm uses its own definition of bool that shouldn't affect the higher-level definition of that type

/dk

ppisar commented 5 months ago

It's not specific to Xpm. Bool is part of X11 public API. In C there is no hierarchy or name spacing in type definitions. If X11 used typedef instead of a macro, you would get a warning/error earlier about type redefinition. Practically speaking X11 is here longer than Prima, so it's Prima that needs to adjust.

One option is a tedious dance with define/undefine around all Bool occurrences. That would made the code very uggly. Another option is to rename Prima's Bool to e.g. PBool through out the code. That's the safest way from backward compatibility point of view. Another option is to replace Prima's Bool with standard C _Bool type. Though it exists only since C99. If you don't like that name, including provides bool alias for that type. Another option, I choose as it was the simplest one, is copy the X11 definition. It can cause an ABI change on some architectures.

dk commented 5 months ago

I dont agree that prima should adhere to x11 definitions because it can also compile on win32. Also, all x11-specific files compile just fine, even though there are no good separation between x11 and prima types.

If x11/xpm.h did not define the bool type as it does, and i wouldn't need to make that horrible hack around it, the issue wouldnt be there at all. I think it has to be solved in xpm.c f ex by declaring the virtual methods with I32 instead of Bool, however horrible that looks. I also don't think that issue is worth creating a type or api that affects the whole toolkit.

dk commented 5 months ago

1.patch

could you possibly check if this patch works for you?

ppisar commented 5 months ago

I tested your patch and fount that it fixes this problem.