-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
Fix Crossplatform mapping for Windows #36
base: master
Are you sure you want to change the base?
Conversation
WalkthroughThis update fine-tunes the mapping between cross-platform Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/windows.rs (1 hunks)
- tests/windows.rs (1 hunks)
Additional comments: 2
tests/windows.rs (1)
- 9-9: The addition of a new test case for a cross-platform value of 80 mapping to
WinAPIThreadPriority::Highest
is a good practice to validate the changes made in the mapping logic. It ensures that the new value is correctly handled and mapped, enhancing the test coverage for the priority mapping functionality.src/windows.rs (1)
- 80-83: The adjustments made to the priority ranges in the
TryFrom<ThreadPriority> for WinAPIThreadPriority
implementation correctly account for the previously missing values (20, 40, 60, and 80) by shifting the ranges forBelowNormal
,Normal
,AboveNormal
, andHighest
priorities. This change ensures a more comprehensive and evenly distributed mapping across Windows thread priority values, aligning with the PR objectives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for finding this :)
When reading through the Crossplatform value mapping for Windows, I noticed that 20, 40, 60, and 80 were within the valid range of [0,99], but were not covered by the match statement. This PR updates the thread priority ranges to include each of these values as appropriate and also adds a test case for Crossplatform value 80. The ranges were adjusted to keep the Crossplatform values distributed as even as possible across the Windows thread priority values.
Summary by CodeRabbit