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

ThreadPriorityValue::MIN/ThreadPriorityValue::MAX have unexpected types #38

Open
nazar-pc opened this issue Feb 28, 2024 · 3 comments
Open

Comments

@nazar-pc
Copy link

nazar-pc commented Feb 28, 2024

I believe those should have ThreadPriorityValue type, not u8. It is always possible to convert it to u8 later if desired, but converting u8 to ThreadPriorityValue requires unwrap() or expect(), which is inconvenient and unnecessary.

P.S. Clippy warning here has nothing to do with From<u8>, it suggests you to do impl From<ThreadPriorityValue> for u8:

thread-priority/src/lib.rs

Lines 264 to 271 in 74cf570

// The From<u8> is unsafe, so there is a TryFrom instead.
// For this reason we silent the warning from clippy.
#[allow(clippy::from_over_into)]
impl std::convert::Into<u8> for ThreadPriorityValue {
fn into(self) -> u8 {
self.0
}
}

@iddm
Copy link
Owner

iddm commented Feb 28, 2024

Thank you for reporting this, I'll have to check. It has been a long time since I last worked on this project. Not to mention that I privately am rewriting it to something much simpler than it is now.

@iddm
Copy link
Owner

iddm commented Mar 28, 2024

So I checked the code, and I remember the reason why I did it this way.

So the ThreadPriorityValue was supposed to be a universal numeric value in something like percentage, varying from 0 to 99 inclusively. The higher the value - the higher the priority should be, and vice versa. The reason I implemented only the Into<u8> and not From<ThreadPriorityValue> is because not all the values of u8 are correct for ThreadPriorityValue, but only in the range of 0..=99, so any value from within the range of 100.==255 will be incorrect. Hence, there was no safe and guaranteed conversion from ThreadPriorityValue to u8; the opposite was true from u8 to ThreadPriorityValue. And this is why there is TryFrom.

I understand that the unwrap() or expect() isn't something one might want to have all the time, but unfortunately, there is no safe way to convert from one to another. We could have some clamping the values over 99, but this might have implicit meaning for some people, who might think that there is indeed a difference between 99 and 100 and any other value higher than 99.

@nazar-pc
Copy link
Author

The reason I implemented only the Into<u8> and not From<ThreadPriorityValue> is because not all the values of u8 are correct for ThreadPriorityValue

What you are talking about is impl From<u8> for ThreadPriorityValue, which if added would be a problem indeed.

What clippy is talking about is impl From<ThreadPriorityValue> for u8, which doesn't violate any expectations and does exactly the same thing as impl Into<u8> for ThreadPriorityValue in the end, just more awkward sometimes.

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

No branches or pull requests

2 participants