Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove redundant sizeof "size_t" check #4676

Merged
merged 1 commit into from
Jun 15, 2022

Conversation

weitjong
Copy link
Contributor

The result variables: HAVE_${VARIABLE}, ${VARIABLE}, ${VARIABLE}_CODE, etc. do not seem to be referenced anywhere in the CMake build script. Unless I was mistaken on how the check is being used.

The motivation for me to have the redundant check removed is because otherwise it would cause an error in my SDL initial build tree generation with my own custom Emscripten.cmake toolchain file. My custom CMake toolchain file is much simpler version than the official one provided by EMSDK, which does not provide the "hack" version of the CheckTypeSize.cmake.

But in any case, there should be no harm to have this redundant check removed for other use cases.

My Web CI build can be found here: https://github.com/alpha-scorpii-b/SDL/runs/3383479743?check_suite_focus=true

The result variables: HAVE_${VARIABLE}, ${VARIABLE}, ${VARIABLE}_CODE,
etc. do not seem to be referenced anywhere in the CMake build script.
@madebr
Copy link
Contributor

madebr commented Jun 15, 2022

SIZEOF_SIZE_T is indeed never used.
It might however be useful to detect CHERI systems, where sizeof(size_t) != sizeof(void*). (I don't know SDL supports any such platform)
See this fmtlib pr for more info.

@icculus icculus merged commit c90e1ec into libsdl-org:main Jun 15, 2022
@icculus
Copy link
Collaborator

icculus commented Jun 15, 2022

This is fine; if we find ourselves on a system that needs it, we'll deal with it.

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