refactor!: Remove lazy static instances #250

leon-xd · 2024-12-04T23:57:41Z

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

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

Cargo.toml

crates/wdk-build/src/lib.rs

crates/wdk-sys/Cargo.toml

Cargo.toml

crates/wdk-sys/build.rs

Copilot

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()")

tests/wdk-macros-tests/tests/outputs/nightly/macrotest/bug_unused_imports.expanded.rs

wmmc88

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:

we do not expose WDF version number in EXPORTED_CFG_SETTINGS in emit_cfg_settings
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

crates/wdk-sys/build.rs

wmmc88 · 2025-01-15T04:22:36Z

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:

we do not expose WDF version number in EXPORTED_CFG_SETTINGS in emit_cfg_settings

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

Co-authored-by: Melvin Wang <[email protected]> Signed-off-by: Leon Durrenberger <[email protected]>

wmmc88 · 2025-01-16T02:18:02Z

crates/wdk-sys/src/wdf.rs

@@ -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"));


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

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

Leon Durrenberger added 7 commits November 19, 2024 12:33

temp commit, moving between devices

ab36865

Finished implementation of FunctionTable

76b6805

reverted changes made by cargo fmt

214e5cb

last cleanup, consolidate imports

fea4dfb

corrected formatting

1f035a8

adapted to cargo clippy

31c883e

fix newline around format macro

0243f68

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

Copilot AI reviewed Dec 5, 2024

View reviewed changes

Taplo passing locally

ea050a6

leon-xd commented Dec 5, 2024

View reviewed changes

crates/wdk-sys/build.rs Outdated Show resolved Hide resolved

leon-xd commented Dec 5, 2024

View reviewed changes

crates/wdk-sys/build.rs Outdated Show resolved Hide resolved

leon-xd commented Dec 5, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

wmmc88 changed the title ~~chore: Remove lazy static instances~~ refactor: Remove lazy static instances Dec 6, 2024

wmmc88 requested changes Dec 6, 2024

View reviewed changes

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

Leon Durrenberger added 4 commits December 11, 2024 14:25

addressed PR comments, removed function table static

b49a18f

passsed all pipeline checks

a8280a3

finished nightly clippy check

dba3ef0

added doc comment for EXPORTED_CFG_SETTINGS

9075140

wmmc88 requested a review from Copilot December 27, 2024 18:25

Copilot AI reviewed Dec 27, 2024

View reviewed changes

tests/wdk-macros-tests/tests/outputs/nightly/macrotest/bug_unused_imports.expanded.rs Show resolved Hide resolved

wmmc88 self-assigned this Jan 10, 2025

wmmc88 requested changes Jan 15, 2025

View reviewed changes

wmmc88 requested review from a team January 15, 2025 20:58

leon-xd changed the title ~~refactor: Remove lazy static instances~~ refactor!: Remove lazy static instances Jan 15, 2025

leon-xd and others added 7 commits January 15, 2025 13:45

Update crates/wdk-sys/build.rs

daac5f5

Co-authored-by: Melvin Wang <[email protected]> Signed-off-by: Leon Durrenberger <[email protected]>

Update crates/wdk-sys/build.rs

a7dce5d

Co-authored-by: Melvin Wang <[email protected]> Signed-off-by: Leon Durrenberger <[email protected]>

Update crates/wdk-sys/build.rs

14f90c4

Co-authored-by: Melvin Wang <[email protected]> Signed-off-by: Leon Durrenberger <[email protected]>

Merge branch 'main' into remove-lazy-static-instances

c817506

addressed small comments

f17f5e7

put comment in right file

1e59238

passing fmt

f735587

wmmc88 reviewed Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: Remove lazy static instances #250

refactor!: Remove lazy static instances #250

leon-xd commented Dec 4, 2024 •

edited by wmmc88

Loading

Copilot AI left a comment

wmmc88 left a comment

wmmc88 commented Jan 15, 2025

wmmc88 Jan 16, 2025

wmmc88 Jan 16, 2025

refactor!: Remove lazy static instances #250

Are you sure you want to change the base?

refactor!: Remove lazy static instances #250

Conversation

leon-xd commented Dec 4, 2024 • edited by wmmc88 Loading

Breaking Changes

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

wmmc88 left a comment

Choose a reason for hiding this comment

wmmc88 commented Jan 15, 2025

wmmc88 Jan 16, 2025

Choose a reason for hiding this comment

wmmc88 Jan 16, 2025

Choose a reason for hiding this comment

leon-xd commented Dec 4, 2024 •

edited by wmmc88

Loading