search

Rust-SDL2 / rust-sdl2

SDL2 bindings for Rust
MIT License
2.65k stars 465 forks source link

Fix some undefined behavior #1389

Closed antonilol closed 2 weeks ago

antonilol commented 2 months ago

review note: read_pixels had some indentation changed, word diff looks better to see what changed there

fix some undefined behavior (not all changes are) f374447d103898542afcc789e0f00d0667759ae2 removes an unnecessary call to an unsafe function and 2accf9909cc3342176084897d4819ef5b08d43b6 fixes a memory leak

Cobrand commented 2 months ago

For the [repr(transparent)], I understand the reason so it's fine (maybe add a comment so that we understand why it's there)

For the flip flag, you should not change the sdl_bindings manually, but I do believe it needs to be changed. You need to change the parameters of binding so that it generates what you want.

Have a look at https://docs.rs/bindgen/0.53.3/bindgen/struct.Builder.html#method.bitfield_enum and sdl2-sys/build.rs where the bindings are generated, to change a specific struct to bitfield like directly within sdl2-sys.

When looking at the bindgen docs, remember that we are quite a few versions behind, it might be a good idea to update at some point, but it does not look like it's necessary here.

Anyway, putting an extern "C" in rust-sdl2 itself is a big code smell and a big no-no.

antonilol commented 2 months ago

For the [repr(transparent)], I understand the reason so it's fine (maybe add a comment so that we understand why it's there)

will add a comment there

When looking at the bindgen docs, remember that we are quite a few versions behind, it might be a good idea to update at some point, but it does not look like it's necessary here.

i could not generate the bindings on the current version of bindgen used here, searching the issue online got me to a closed issue on the bindgen repo, the fix was included in 0.69 and there it works for me for the sdl_bindings.rs, but not yet for the image, ttf and gfx ones because those have different versions? from this code i assumed sdl version 2.0.20 was used, but i could not find that version number in all of the other libraries

antonilol commented 2 months ago

i reverted the fix to copy_ex for now, as it would probably be needed (and at least easier for me) to do this after updating bindgen, and also to wrap up this pr

antonilol commented 2 months ago

if you have no comments/question left on this pr you may merge it

antonilol commented 2 months ago

i started with updating bindgen here: https://github.com/antonilol/rust-sdl2/tree/update-bindgen, if you wanted to do this already you can use that as a starting point

how did you generate the bindings yourself? like from which header files, which versions of software used. i assume you did not get an error on the current version of bindgen used in the -sys crate