libsdl-org / sdl12-compat

An SDL-1.2 compatibility layer that uses SDL 2.0 behind the scenes.
Other
191 stars 40 forks source link

Advanced Strategic Command (asc) fails to build against sdl12-compat headers #299

Closed smcv closed 1 year ago

smcv commented 1 year ago

To reproduce:

Expected result: both calls to dpkg-buildpackage succeed.

Actual result:

The first build (against classic SDL 1.2) works.

The second build fails:

/bin/bash ../../../../libtool  --tag=CXX   --mode=compile g++ -DHAVE_CONFIG_H -I. -I../../../..   -Wdate-time -D_FORTIFY_SOURCE=2 -I/usr/lib/x86_64-linux-gnu/wx/include/gtk3-unicode-3.2 -I/usr/include/wx-3.2 -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL -D__WXGTK__ -I/usr/include/SDL -D_GNU_SOURCE=1 -D_REENTRANT  -g -O2 -ffile-prefix-map=/home/smcv/tmp/asc=. -fstack-protector-strong -Wformat -Werror=format-security -std=c++11 -Wno-sign-compare -D_UNIX_ -D_SDL_ -I/usr/include/freetype2 -I/usr/include/libpng16   -DSIZE_T_not_identical_to_INT -c -o sdlmm_pixelformat.lo sdlmm_pixelformat.cpp
In file included from sdlmm_display.h:25,
                 from sdlmm_display.cpp:25:
sdlmm_basesurface.h: In member function 'SDLmm::private_hwdata* SDLmm::BaseSurface::hwdata() const':
sdlmm_basesurface.h:142:66: error: invalid conversion from 'void*' to 'SDLmm::private_hwdata*' [-fpermissive]
  142 |     struct private_hwdata *hwdata() const { return GetSurface()->hwdata; }
      |                                                    ~~~~~~~~~~~~~~^~~~~~
      |                                                                  |
      |                                                                  void*

I think this is because classic SDL 1.2 has struct private_hwdata *hwdata as the 8th member of struct SDL_Surface, but sdl12-compat simplifies that to void *hwdata.

That simplification is obviously fine from an ABI point of view, because both are data pointers.

From an API point of view, it's a valid simplification in C, because void * in C isn't very type-safe: after declaring void *generic; SomeType *typed it's valid to say either generic = typed or typed = generic.

But C++ is more type-safe, and will only allow one-way assignment without a cast: generic = typed is still OK, but typed = generic isn't allowed (and you would have to use typed = (SomeType *) generic or typed = static_cast<SomeType*>(generic) instead).

smcv commented 1 year ago

More minimal reproducer:

#include <SDL.h>

extern "C" int main (void)
{
    SDL_Surface foo;
    struct private_hwdata *hwdata;

    hwdata = foo.hwdata;
    return 0;
}

g++ -ot t.cpp -I .../libsdl1.2/include (with classic headers) ⇒ works

g++ -ot t.cpp $(pkg-config --cflags --libs sdl) (with sdl12-compat headers) ⇒ fails

g++ -ot t.cpp -I .../sdl12-compat/include/SDL (with sdl12-compat headers) ⇒ fails

g++ -ot t.cpp -I .../sdl12-compat/include/SDL (with the PR I'm about to propose) ⇒ works

smcv commented 1 year ago

I'm in two minds about whether this is a compatibility bug in sdl12-compat, or a bug in the SDLmm C++ binding that's bundled in the asc package (https://sources.debian.org/src/asc/2.6.1.0-9/source/libs/sdlmm/ which appears to be a copy of https://sourceforge.net/projects/sdlmm/).

If I understand the API correctly, the struct private_hwdata is exclusively for internal use by SDL, and therefore there's no legitimate reason for third-party code to be looking at that member at all, so it's pointless for SDLmm::BaseSurface to have an accessor for it.

But on the other hand, sdl12-compat is aiming for 100% compatibility with classic SDL 1.2 - does this include misfeatures like this one?

icculus commented 1 year ago

(In sdl2-compat, we likely won't have problems like this, since we could copy the original SDL2 headers over wholesale. In sdl12-compat, we were trying to avoid concerns that the original headers were LGPL-licensed, and the ramifications of that, by writing the most minimal equivalent from scratch.)