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

refactor!: Remove lazy static instances #250

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

leon-xd
Copy link

@leon-xd leon-xd commented Dec 4, 2024

Replaced lazy static instances with std::sync::LazyLock in following locations:

  • crates/wdk-build/src/lib.rs
  • crates/wdk-sys/build.rs
  • tests/wdk-macros-tests/src/lib.rs

Replaced lazy static instances with custom FunctionTable struct in generated code in crates/wdk-sys/build.rs

Breaking Changes

  • Removes wdf_function_table.rs from generated code of WDK_BUILD. Users should access the function table instead via pointer at wdk_sys::WdfFunctions, and checking bounds via wdk_sys::wdf::get_wdf_function_count() which is generated in wdf_function_count.rs
  • lazy-static is no longer dependency in the workspace and is replaced with sync::LazyLock as introduced in Rust 1.83

closes #192

@wmmc88 wmmc88 requested review from Copilot, NateD-MSFT and wmmc88 and removed request for NateD-MSFT December 5, 2024 00:58

Choose a reason for hiding this comment

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

Copilot reviewed 15 out of 17 changed files in this pull request and generated no suggestions.

Files not reviewed (2)
  • crates/wdk-sys/Cargo.toml: Evaluated as low risk
  • crates/wdk-build/Cargo.toml: Evaluated as low risk
crates/wdk-sys/build.rs Outdated Show resolved Hide resolved
crates/wdk-sys/build.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@wmmc88 wmmc88 changed the title chore: Remove lazy static instances refactor: Remove lazy static instances Dec 6, 2024
crates/wdk-build/src/lib.rs Show resolved Hide resolved
crates/wdk-sys/Cargo.toml Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
crates/wdk-sys/build.rs Outdated Show resolved Hide resolved
crates/wdk-sys/build.rs Outdated Show resolved Hide resolved
@wmmc88 wmmc88 requested a review from Copilot December 27, 2024 18:25
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 29 out of 44 changed files in this pull request and generated 1 comment.

Files not reviewed (15)
  • tests/wdk-macros-tests/tests/outputs/beta/macrotest/bug_tuple_struct_shadowing.expanded.rs: Evaluated as low risk
  • tests/wdk-macros-tests/tests/outputs/beta/macrotest/bug_unused_imports.expanded.rs: Evaluated as low risk
  • crates/wdk-sys/src/lib.rs: Evaluated as low risk
  • crates/wdk-build/Cargo.toml: Evaluated as low risk
  • Cargo.toml: Evaluated as low risk
  • crates/wdk-build/src/lib.rs: Evaluated as low risk
  • tests/wdk-macros-tests/Cargo.toml: Evaluated as low risk
  • crates/wdk-sys/src/wdf.rs: Evaluated as low risk
  • tests/wdk-macros-tests/src/lib.rs: Evaluated as low risk
  • crates/wdk-sys/Cargo.toml: Evaluated as low risk
  • crates/wdk-sys/build.rs: Evaluated as low risk
  • tests/wdk-macros-tests/tests/outputs/beta/macrotest/wdf_verifier_dbg_break_point.expanded.rs: Evaluated as low risk
  • tests/wdk-macros-tests/tests/outputs/beta/macrotest/wdf_spin_lock_acquire.expanded.rs: Evaluated as low risk
  • tests/wdk-macros-tests/tests/outputs/beta/macrotest/wdf_request_retrieve_output_buffer.expanded.rs: Evaluated as low risk
  • tests/wdk-macros-tests/tests/outputs/nightly/macrotest/bug_tuple_struct_shadowing.expanded.rs: Evaluated as low risk
Comments suppressed due to low confidence (1)

tests/wdk-macros-tests/tests/outputs/beta/macrotest/wdf_device_create.expanded.rs:30

  • [nitpick] The error message is too verbose. Consider simplifying it to: 'assertion failed: invalid function count size'.
::core::panicking::panic("assertion failed: isize::try_from(wdf_function_count *\n            core::mem::size_of::<wdk_sys::WDFFUNC>()).is_ok()")

@wmmc88 wmmc88 self-assigned this Jan 10, 2025
Copy link
Collaborator

@wmmc88 wmmc88 left a comment

Choose a reason for hiding this comment

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

I really dislike adding a public api for the function count, since no such public api exists in the WDK, and i'd like for the sys crate to be purely the wdk apis. Ideally, the proc_macro should generate code that either dereferences the symbol OR uses the enum by conditionally compiling for the relevant cases. This essentially means turning the is_wdf_function_count_generated expression into a CFG.

This is currently not possible since:

  1. we do not expose WDF version number in EXPORTED_CFG_SETTINGS in emit_cfg_settings
  2. it would be very tedious and manual to cfg over all the supported values, since there is no rust cfg equivalent to UMDF_VERSION_MINOR > 1234. We would need to figure out a way to write a cfg in this fashion (is this even possible?)

I think those things are out of the scope of this PR, and the best we can do right now is hide the get_wdf_function_count api from the user and make it clear that it is subject to future removal once we've figured out the above.

crates/wdk-sys/src/lib.rs Show resolved Hide resolved
crates/wdk-sys/build.rs Outdated Show resolved Hide resolved
crates/wdk-sys/build.rs Show resolved Hide resolved
crates/wdk-sys/build.rs Outdated Show resolved Hide resolved
crates/wdk-sys/build.rs Outdated Show resolved Hide resolved
crates/wdk-sys/build.rs Show resolved Hide resolved
@wmmc88
Copy link
Collaborator

wmmc88 commented Jan 15, 2025

I really dislike adding a public api for the function count, since no such public api exists in the WDK, and i'd like for the sys crate to be purely the wdk apis. Ideally, the proc_macro should generate code that either dereferences the symbol OR uses the enum by conditionally compiling for the relevant cases. This essentially means turning the is_wdf_function_count_generated expression into a CFG.

This is currently not possible since:

  1. we do not expose WDF version number in EXPORTED_CFG_SETTINGS in emit_cfg_settings
  2. it would be very tedious and manual to cfg over all the supported values, since there is no rust cfg equivalent to UMDF_VERSION_MINOR > 1234. We would need to figure out a way to write a cfg in this fashion (is this even possible?)

I think those things are out of the scope of this PR, and the best we can do right now is hide the get_wdf_function_count api from the user and make it clear that it is subject to future removal once we've figured out the above.

For more on #2, see https://users.rust-lang.org/t/cfg-w-arbitrary-expressions/109371

I think it may be possible to write a new attribute macro that expands into a cfg

@wmmc88 wmmc88 requested review from a team January 15, 2025 20:58
@leon-xd leon-xd changed the title refactor: Remove lazy static instances refactor!: Remove lazy static instances Jan 15, 2025
@@ -20,4 +20,4 @@ mod bindings {
include!(concat!(env!("OUT_DIR"), "/wdf.rs"));
}

include!(concat!(env!("OUT_DIR"), "/wdf_function_table.rs"));
include!(concat!(env!("OUT_DIR"), "/wdf_function_count.rs"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put this inside a module named __private, and have the module be #[doc(hidden)]. I don't like our sys crate having a public api that isn't present in the WDK

Copy link
Collaborator

Choose a reason for hiding this comment

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

to be clear, this doesn't actually make it private. the modules still needs to be pub in order for the proc_macro to access it. This just hides it from the generated docs (and rust-analyzer auto-complete) and makes it clear that its an internal implementation detail.

Similar approaches are used in crates like serde to expose internal-only apis to their proc_macros: https://github.com/serde-rs/serde/blob/930401b0dd58a809fce34da091b8aa3d6083cb33/serde/src/lib.rs#L324

We also use a similar approach in our proc_macro

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.

Remove all lazy_static instances
2 participants