libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
10k stars 1.85k forks source link

XKeycodeToKeysym deprecation message when building with -DSDL_X11_SHARED=OFF #7755

Closed madebr closed 1 year ago

madebr commented 1 year ago

When configuring SDL with -DSDL_X11_SHARED=OFF, a deprecation message appears:

[257/641] Building C object CMakeFiles/SDL3-shared.dir/src/video/x11/SDL_x11dyn.c.o
In file included from /home/maarten/projects/SDL/src/video/x11/SDL_x11dyn.c:189:
/home/maarten/projects/SDL/src/video/x11/SDL_x11sym.h: In function ‘SDL_X11_LoadSymbols’:
/home/maarten/projects/SDL/src/video/x11/SDL_x11sym.h:198:1: warning: ‘XKeycodeToKeysym’ is deprecated [-Wdeprecated-declarations]
  198 | SDL_X11_SYM(KeySym,XKeycodeToKeysym,(Display* a,KeyCode b,int c),(a,b,c),return)
      | ^~~~~~~~~~~
In file included from /home/maarten/projects/SDL/src/video/x11/SDL_x11dyn.h:26,
                 from /home/maarten/projects/SDL/src/video/x11/SDL_x11dyn.c:27:
/usr/include/X11/Xlib.h:1687:15: note: declared here
 1687 | extern KeySym XKeycodeToKeysym(
      |               ^~~~~~~~~~~~~~~~
Kontrabant commented 1 year ago

It seems that it was marked deprecated in favor of XkbKeycodeToKeysym due to a bug that was fixed upstream years later: https://gitlab.freedesktop.org/xorg/lib/libx11/-/issues/97

The X backend already uses the XkbKeycodeToKeysym function when it can, but it still has a path that falls back to the deprecated legacy function if the xkb extension isn't supported by the X server.

https://github.com/libsdl-org/SDL/blob/8096f7269b0b0692bdb08cf0f41e411ae9203619/src/video/x11/SDL_x11keyboard.c#LL95C1-L131C1

I don't think there is a way to fix this without relegating the legacy case to strictly being a compile-time check, as the deprecated function is otherwise required to be present.

slouken commented 1 year ago

Since we have properly handled the deprecation, we can probably fix the warning with a #pragma

slouken commented 1 year ago

@madebr, can you take care of that?

madebr commented 1 year ago

Fixed in 23db9716814bfad469227a5d7069dc1898310d96 (SDL3) and d38ccfa88f2e1c5912546ca026dda0ae7948d2bc (SDL2)

sezero commented 1 year ago

Fixed in 23db971 (SDL3) and d38ccfa (SDL2)

I'm getting the following error from my old gcc after those commits:

In file included from /tmp/SDL3/src/video/x11/SDL_x11dyn.c:127:
/tmp/SDL3/src/video/x11/SDL_x11sym.h: In function ‘SDL_X11_UnloadSymbols’:
/tmp/SDL3/src/video/x11/SDL_x11sym.h:198: error: #pragma GCC diagnostic not allowed inside functions
/tmp/SDL3/src/video/x11/SDL_x11sym.h:199: error: #pragma GCC diagnostic not allowed inside functions
/tmp/SDL3/src/video/x11/SDL_x11sym.h:207: error: #pragma GCC diagnostic not allowed inside functions
In file included from /tmp/SDL3/src/video/x11/SDL_x11dyn.c:163:
/tmp/SDL3/src/video/x11/SDL_x11sym.h: In function ‘SDL_X11_LoadSymbols’:
/tmp/SDL3/src/video/x11/SDL_x11sym.h:198: error: #pragma GCC diagnostic not allowed inside functions
/tmp/SDL3/src/video/x11/SDL_x11sym.h:199: error: #pragma GCC diagnostic not allowed inside functions
/tmp/SDL3/src/video/x11/SDL_x11sym.h:207: error: #pragma GCC diagnostic not allowed inside functions
In file included from /tmp/SDL3/src/video/x11/SDL_x11dyn.c:167:
/tmp/SDL3/src/video/x11/SDL_x11sym.h:198: error: #pragma GCC diagnostic not allowed inside functions
/tmp/SDL3/src/video/x11/SDL_x11sym.h:199: error: #pragma GCC diagnostic not allowed inside functions
/tmp/SDL3/src/video/x11/SDL_x11sym.h:207: error: #pragma GCC diagnostic not allowed inside functions

Can we not live with that warning?

madebr commented 1 year ago

I chose to to the #pragma push/#pragma pop "surgically" around the troublesome XKeycodeToKeysym.

The push/pop can also be done around SDL_X11_LoadSymbols. The downside of this is that all deprecation warnings will be silenced. Alternatively, replace #if defined(__GNUC__) || defined(__clang__) with #if (defined(__GNUC__) && __GNUC__ >= XXXX) || defined(__clang__)?

sezero commented 1 year ago

The push/pop can also be done around SDL_X11_LoadSymbols. The downside of this is that all deprecation warnings will be silenced.

That would be undesirable

Alternatively, replace #if defined(__GNUC__) || defined(__clang__) with #if (defined(__GNUC__) && __GNUC__ >= XXXX) || defined(__clang__)?

I can't remember since which version gcc began accepting the pragma within a function body..

Alternatively: Just drop the patch and live with that corner case warning

madebr commented 1 year ago

I can't remember since which version gcc began accepting the pragma within a function body..

With godbolt, I found out 4.5.3 errors, and 4.6.4 succeeds. So what about the following?

#if (defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6))) || defined(__clang__)
sezero commented 1 year ago

I can't remember since which version gcc began accepting the pragma within a function body..

With godbolt, I found out 4.5.3 errors, and 4.6.4 succeeds. So what about the following?

#if (defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6))) || defined(__clang__)

That's OK with me

sezero commented 1 year ago

I can't remember since which version gcc began accepting the pragma within a function body..

With godbolt, I found out 4.5.3 errors, and 4.6.4 succeeds. So what about the following?

#if (defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6))) || defined(__clang__)

That's OK with me

And please remember to apply to both SDL2 and SDL3

madebr commented 1 year ago

2d6bae70b4374a96e0d69c2396a3182d6973afff (SDL3) and 4a3a9f3ad89d211c9b55e8bddbad30e32e10bc8d (SDL2)