Skip to content

Conversation

@icculus
Copy link
Collaborator

@icculus icculus commented Oct 13, 2025

Rather than have another place we generate this list, let's use the one we already have!

This saves problems on older PRs that predate testsymbols, like #13264, and also prevents little mistakes, like SDL_SetX11EventHook being in the list twice.

@icculus icculus added this to the 3.4.0 milestone Oct 13, 2025
@icculus icculus requested review from madebr and slouken October 13, 2025 14:07
@icculus icculus self-assigned this Oct 13, 2025
@icculus icculus merged commit 72a3e40 into libsdl-org:main Oct 13, 2025
43 checks passed
@icculus icculus deleted the sdl3-testsymbols-from-dynapi branch October 13, 2025 14:58
@madebr
Copy link
Contributor

madebr commented Oct 13, 2025

The reason for testsymbols to exist (existed?) is (was?) to make sure non-dynapi ports have all symbols available.
We recently saw an error with ngage missing an implementation which would've been caught earlier by testsymbols.

@icculus
Copy link
Collaborator Author

icculus commented Oct 13, 2025

It still does that, though...? It just gets the list of symbols from a different place, but it will work whether the port uses dynapi or not.

@icculus
Copy link
Collaborator Author

icculus commented Oct 13, 2025

(If I'm wrong, though, let's fix this and/or revert it.)

@madebr
Copy link
Contributor

madebr commented Oct 13, 2025

It still does that, though...? It just gets the list of symbols from a different place, but it will work whether the port uses dynapi or not.

dynapi will totally complain about missing symbols for the (desktop-only) platforms that use it
This was the (stop-gap) fix for N-Gage: SDL_TimeToDateTime is a platform-specific function.

An alternative to testsymbols could be to make sure every SDL3 symbol is used at least once by a test.

@icculus
Copy link
Collaborator Author

icculus commented Oct 13, 2025

Yes, but it's just using the dynapi symbol list to generate the same C code that was previously in testsymbols.c, so it should still catch things like the ngage problem. The intention is this program is exactly the same, it just doesn't manage this list of symbols separately.

I feel like you're saying something very clearly that I'm totally misunderstanding anyhow here. :)

@madebr
Copy link
Contributor

madebr commented Oct 13, 2025

I feel like you're saying something very clearly that I'm totally misunderstanding anyhow here. :)

Nah, you are correct. I misread the PR in the GitHub UI. I thought you removed testsymbols.c, but you didn't.
We're all good :)

Sorry about the misunderstanding!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants