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

testnative: fix when building with -DSDL_LIBC=OFF #10062

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

madebr
Copy link
Contributor

@madebr madebr commented Jun 19, 2024

SDL_rand returns unsigned integers,
so SDL_rand() % (MAX_SPEED * 2 + 1)) - MAX_SPEED (MAX_SPEED = 1) can result in 2**32-1.

I also included a bonus fix for testnative.c that failed to link on Windows when built with -DSDL_LIBC=OFF.
Including build_config/SDL_build_config.h directly was simpler then copying the testevdev workaround to include SDL_internal.h.

Description

Existing Issue(s)

include/SDL3/SDL_stdinc.h Outdated Show resolved Hide resolved
@madebr madebr marked this pull request as draft June 19, 2024 17:50
@JKaniarz
Copy link
Contributor

JKaniarz commented Jun 19, 2024

See my comments on pr #10063

SDL_rand() % n is a common, but bad, way of getting a random number < n. That's why I added SDL_rand_n() in my PR.

I know I'm deviating from std::rand() by returning a Uint32. I did update the docs to say it returns 32 random bits (instead of a number) and suggest using SDL_rand_n(). Here are the pros/cons:

  • Returning 31 random bits is the norm, and predicability is good.
  • Returning 31 random bits makes doing things right more complicated and people will default to the bad SDL_rand()%n .

There's an open question on the PR about if SDL_rand_n() should use Sint32 even though negative numbers don't quite work right.

@slouken
Copy link
Collaborator

slouken commented Jun 19, 2024

SDL_rand_n() should probably take and return Sint32, but document that the valid range of n is [0...SDL_MAX_SINT32], otherwise you'll run into problems mixing SDL_rand_n() with signed arithmetic.

@slouken
Copy link
Collaborator

slouken commented Jun 19, 2024

We should probably have SDL_RAND_MAX, for consistency. :)

@JKaniarz
Copy link
Contributor

SDL_rand_n() should probably take and return Sint32, but document that the valid range of n is [0...SDL_MAX_SINT32], otherwise you'll run into problems mixing SDL_rand_n() with signed arithmetic.

Will do.

@slouken
Copy link
Collaborator

slouken commented Jun 19, 2024

I agree that SDL_rand() should return 32-bits, so we have the full 32-bits of values for _float(), etc... actually, float can't represent the full range of 32-bit values, maybe using 31 bits would be fine?

@JKaniarz
Copy link
Contributor

JKaniarz commented Jun 19, 2024

What if we rename SDL_rand() to SDL_rand_bits(), and make SDL_rand() take an n parameter so nobody can accidentally use it like std::rand? And forget SDL_RAND_MAX because you manually provide the max to rand.

Edit: I really like the rand_bits() idea. But maybe we can keep the std::rand() interface for SDL_rand().
Edit Edit: But I really really want to stop people from doing SDL_rand() % n

@slouken
Copy link
Collaborator

slouken commented Jun 19, 2024

Maybe we don't expose the rand() interface at all? We just have rand_n and rand_float?

@JKaniarz
Copy link
Contributor

Maybe we don't expose the rand() interface at all? We just have rand_n and rand_float?

Yeah. If we're not going to implement std::rand() with all its warts, we probably shouldn't mimic its interface.

@JKaniarz
Copy link
Contributor

Just submitted #10064

Deleted SDL_rand() to force users onto the correct functions.
SDL_rand_bits() is Uint32.
SDL_rand_n(n) is Sint32

@JKaniarz
Copy link
Contributor

JKaniarz commented Jun 19, 2024

@madebr With the changes on #10064, I think commit 61a58a8 is no longer necessary (and will also cause a merge conflict). Does the PR address your concern?

@madebr
Copy link
Contributor Author

madebr commented Jun 19, 2024

@madebr With the changes on #10064, I think commit 61a58a8 is no longer necessary (and will also cause a merge conflict). Does the PR address your concern?

Yes! I just tried your pr and testnative behaves fine.
Thanks for the improvements!

(src/dynapi/SDL_dynapi.sym still contains SDL_rand_r and is slightly unsorted around SDL_rand)

@madebr madebr marked this pull request as ready for review June 19, 2024 21:21
@madebr madebr changed the title testnative: SDL_rand requires cast to signed integer testnative: fix when building with -DSDL_LIBC=OFF Jun 19, 2024
@madebr madebr merged commit 72d5f39 into libsdl-org:main Jun 20, 2024
39 checks passed
@madebr madebr deleted the test-simplifications branch June 20, 2024 13:40
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