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

Make BOOL a core type #3441

Merged
merged 6 commits into from
Jan 14, 2025
Merged

Make BOOL a core type #3441

merged 6 commits into from
Jan 14, 2025

Conversation

kennykerr
Copy link
Collaborator

@kennykerr kennykerr commented Jan 14, 2025

This update elevates BOOL to a "core type" in the windows-bindgen crate. Consider the following simple example generating bindings for the EnableMouseInPointer function from a build script.

fn main() {
    windows_bindgen::bindgen(
        "--in default --out src/bindings.rs --flat --filter EnableMouseInPointer --sys"
            .split_whitespace(),
    );
}
windows_targets::link!("user32.dll" "system" fn EnableMouseInPointer(fenable: BOOL) -> BOOL);
pub type BOOL = i32;

That's pretty simple. The windows-targets crate provides linker support but otherwise these bindings are dependency-free. But what if I remove the --sys option?

"--in default --out src/bindings.rs --flat --filter EnableMouseInPointer"

#[inline]
pub unsafe fn EnableMouseInPointer(fenable: bool) -> windows_core::Result<()> {
    windows_targets::link!("user32.dll" "system" fn EnableMouseInPointer(fenable: BOOL) -> BOOL);
    unsafe { EnableMouseInPointer(fenable.into()).ok() }
}
#[must_use]
#[repr(transparent)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, Default)]
pub struct BOOL(pub i32);

This looks mostly right. The richer bindings take care of a bit of type conversion and error handling. The trouble is that the wrapper function expects the BOOL type to provide an ok method for retrieving the Win32 error code and converting it to a Result value. The parameter also expects an Into implementation to convert from bool to BOOL.

One option is to use the new --reference option to explicitly tell windows-bindgen where to find the BOOL implementation rather than simply generating it as a simple struct.

"--in default --out src/bindings.rs --flat --filter EnableMouseInPointer --reference windows,skip-root,Windows"

#[inline]
pub unsafe fn EnableMouseInPointer(fenable: bool) -> windows_core::Result<()> {
    windows_targets::link!("user32.dll" "system" fn EnableMouseInPointer(fenable: windows::Win32::Foundation::BOOL) -> windows::Win32::Foundation::BOOL);
    unsafe { EnableMouseInPointer(fenable.into()).ok() }
}

This works, but does require that you add a dependency on the windows crate with the "Win32_Foundation" feature enabled. The main drawback here is the sheer size of the windows crate. Its great for apps but doesn't work well as a dependency of a library crate that isn't entirely focused on Windows development.

Well by elevating BOOL to a core type, the following produce bindings that only depend on the tiny windows-core crate and avoids the windows crate entirely.

"--in default --out src/bindings.rs --flat --filter EnableMouseInPointer"

#[inline]
pub unsafe fn EnableMouseInPointer(fenable: bool) -> windows_core::Result<()> {
    windows_targets::link!("user32.dll" "system" fn EnableMouseInPointer(fenable: windows_core::BOOL) -> windows_core::BOOL);
    unsafe { EnableMouseInPointer(fenable.into()).ok() }
}

I considered a few other approaches but ultimately BOOL is just too foundational and important of a Windows type and this "just works" for the most common scenarios.

Fixes: #3439

@kennykerr kennykerr requested review from Copilot and riverar January 14, 2025 19:34

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 394 out of 409 changed files in this pull request and generated no comments.

Files not reviewed (15)
  • crates/libs/sys/src/Windows/Wdk/Graphics/Direct3D/mod.rs: Evaluated as low risk
  • crates/libs/sys/src/Windows/Win32/Devices/SerialCommunication/mod.rs: Evaluated as low risk
  • crates/libs/sys/src/Windows/Win32/Data/HtmlHelp/mod.rs: Evaluated as low risk
  • crates/libs/sys/src/Windows/Win32/Devices/Sensors/mod.rs: Evaluated as low risk
  • crates/libs/sys/src/Windows/Win32/Devices/HumanInterfaceDevice/mod.rs: Evaluated as low risk
  • crates/libs/sys/src/Windows/Win32/Devices/Usb/mod.rs: Evaluated as low risk
  • crates/libs/sys/src/Windows/Win32/Devices/AllJoyn/mod.rs: Evaluated as low risk
  • crates/libs/sys/src/Windows/Win32/Devices/Display/mod.rs: Evaluated as low risk
  • crates/libs/sys/src/Windows/Win32/Devices/Communication/mod.rs: Evaluated as low risk
  • crates/libs/sys/src/Windows/Win32/Devices/Enumeration/Pnp/mod.rs: Evaluated as low risk
  • crates/libs/sys/src/Windows/Win32/Data/RightsManagement/mod.rs: Evaluated as low risk
  • crates/libs/sys/src/Windows/Win32/Devices/WebServicesOnDevices/mod.rs: Evaluated as low risk
  • crates/libs/sys/src/Windows/Win32/Devices/Bluetooth/mod.rs: Evaluated as low risk
  • crates/libs/sys/src/Windows/Win32/Devices/BiometricFramework/mod.rs: Evaluated as low risk
  • crates/libs/bindgen/src/writer/cpp_handle.rs: Evaluated as low risk
Comments suppressed due to low confidence (1)

crates/libs/result/src/bool.rs:7

  • The behavior of the newly added BOOL type and its methods is not explicitly covered by tests.
pub struct BOOL(pub i32);
@kennykerr kennykerr merged commit 94e56c9 into master Jan 14, 2025
75 checks passed
@kennykerr kennykerr deleted the bool-core branch January 14, 2025 20:12
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.

windows-bindgen assumes BOOL has ok() method
2 participants