fix panic render_ex

[jagprog5]

closes #1504

preemptive: if sdl_bindings.rs shouldn't be modified manually then please suggest an alternative. note that this is a tricky? case

[antonilol]

preemptive: if sdl_bindings.rs shouldn't be modified manually then please suggest an alternative. note that this is a tricky? case

sdl_bindings.rs shouldn't be modified, it is generated and adds a lot of work if we need to do manual changes everytime we generate it again.

SDL_RendererFlip should not generate a Rust enum, enum members in C are just constants and the enum type itself an alias for int.
enum style is configured in bindgen here:

rust-sdl2/sdl2-sys/build.rs

Line 660 in 1fac15b

.default_enum_style(bindgen::EnumVariation::Rust {

We should at least not make SDL_RendererFlip a Rust enum, but I would go further not use Rust enum style at all in bindings, these are bindings to the C code so the function signatures should be as close as possible to the C function signatures in the header files. SDL_RendererFlip should be c_int (std::ffi::c_int) or any alias or repr(transparent) wrapper, because the C standard says that enums are int.

In the general case you can't fix an enum that is actually used as bit flags by adding a new variant. What if the C enum has many more variants (each a different bit and they can be combined)? It would be nice have a type that restricts values to valid bit flags but currently this would require enums with hundreds of variants, so just using c_int is also for consistency.

[jagprog5]

thanks. that follows what I mentioned in the issue, regarding what sdl3-sys does.

I looked through the rest of sdl_bindings.rs and found that everything else was ok, except for SDL_Keymod:

bitflags! {
    pub struct Mod: u16 {
        const NOMOD = 0x0000;
        const LSHIFTMOD = 0x0001;
        const RSHIFTMOD = 0x0002;
        const LCTRLMOD = 0x0040;
        const RCTRLMOD = 0x0080;
        const LALTMOD = 0x0100;
        const RALTMOD = 0x0200;
        const LGUIMOD = 0x0400;
        const RGUIMOD = 0x0800;
        const NUMMOD = 0x1000;
        const CAPSMOD = 0x2000;
        const MODEMOD = 0x4000;
        const RESERVEDMOD = 0x8000;
    }
}

#[doc(alias = "SDL_SetModState")]
pub fn set_mod_state(&self, flags: Mod) {
    unsafe {
        // BOOM
        sys::SDL_SetModState(transmute::<u32, sys::SDL_Keymod>(flags.bits() as u32));
    }
}

Honourable mention for SDL_WindowFlags, which we convert to an int before ORing, so it's ok.

---

Perhaps: we can update bindgen to change the representation of SDL_RendererFlip and SDL_Keymod enums only.

[antonilol]

SDL_WindowFlags is a weird case, the functions that use it take u32, not SDL_WindowFlags like:
https://github.com/Rust-SDL2/rust-sdl2/blob/1fac15ba89a2c0a8735d6725ae74e9bb57c4457b/sdl2-sys/sdl_bindings.rs#L6981
https://wiki.libsdl.org/SDL2/SDL_CreateWindow

Perhaps: we can update bindgen to change the representation of SDL_RendererFlip and SDL_Keymod enums only.

That keymod thing is definitely UB too and should be fixed.

I just found this: https://docs.rs/bindgen/latest/bindgen/struct.Builder.html#method.bitfield_enum, this fits exactly I would say. I don't know exactly what code it generates.

[jagprog5]

22098d1

[antonilol]

can you try bitfield_enum instead of constified_enum

[jagprog5]

thanks for sharing bitfield_enum.

looks pretty cool. took a look at the generated code with that instead and it looks like:

impl SDL_RendererFlip {
    #[doc = "< Do not flip"]
    pub const SDL_FLIP_NONE: SDL_RendererFlip = SDL_RendererFlip(0);
}
impl SDL_RendererFlip {
    #[doc = "< flip horizontally"]
    pub const SDL_FLIP_HORIZONTAL: SDL_RendererFlip = SDL_RendererFlip(1);
}
impl SDL_RendererFlip {
    #[doc = "< flip vertically"]
    pub const SDL_FLIP_VERTICAL: SDL_RendererFlip = SDL_RendererFlip(2);
}
impl ::core::ops::BitOr<SDL_RendererFlip> for SDL_RendererFlip {
    type Output = Self;
    #[inline]
    fn bitor(self, other: Self) -> Self {
        SDL_RendererFlip(self.0 | other.0)
    }
}
impl ::core::ops::BitOrAssign for SDL_RendererFlip {
    #[inline]
    fn bitor_assign(&mut self, rhs: SDL_RendererFlip) {
        self.0 |= rhs.0;
    }
}
...

imo not necessary since we are are mapping that to public API Mod which does the bitflags over there instead

[antonilol]

imo not necessary since we are are mapping that to public API Mod which does the bitflags over there instead

right

I do like that the constants are defined in an impl of a new type, can you try newtype_enum then?

[jagprog5]

4d61d9a

ok so I'm using bitfield for flip, because I need to do OR when using. and newtype_enum for KeyMod, since we do the OR with the public API (not needed in the binds)

[antonilol]

LGTM

there is a lot of "useless diff" in sdl_bindings.rs which I aim to fix in #1506, will ignore that here