Enforce OS errors are in the allowed range.#441
Conversation
src/error.rs
Outdated
| pub const CUSTOM_START: u32 = (1 << 31) + (1 << 30); | ||
|
|
||
| #[cold] | ||
| pub(super) fn new_os(code: u32) -> Self { |
There was a problem hiding this comment.
In the future we might consider adding a public new_custom constructor and then deprecating the From<NonZeroU32> that does the non-range-checked conversion.
53d8f46 to
0af5ac5
Compare
|
I updated this to be cleaner. I also removed the |
|
What about my suggestion above? |
Sorry, I don't see it. Maybe it is still a draft? |
|
Oh, oops. You are right. |
src/error.rs
Outdated
|
|
||
| #[allow(dead_code)] | ||
| #[cold] | ||
| pub(super) fn new_os(code: u32) -> Self { |
There was a problem hiding this comment.
Could you add some docs for this method?
I also think we can write it like this:
pub(crate) fn from_os_code(code: impl TryInto<u32>) -> Self {
code.try_into()
.ok()
.and_then(NonZeroU32::new)
.and_then(|code| {
if code.get() < Self::INTERNAL_START {
Some(Self(code))
} else {
None
}
})
.unwrap_or(Self::UNEXPECTED)
}This will make backend code a bit nicer.
I don't think we need to distinguish between ERRNO_NOT_POSITIVE and UNEXPECTED. Both cases should never happen in practice in the first place. Plus the former was used only for codes from errno.
There was a problem hiding this comment.
Could you add some docs for this method?
OK. But...
pub(crate) fn from_os_code(code: impl TryInto) -> Self {
Unfortunately that would allow passing a usize or similar, which really shouldn't be implicitly converted. So I think we should not do that.
I don't think we need to distinguish between ERRNO_NOT_POSITIVE and UNEXPECTED
I am OK with making that change.
There was a problem hiding this comment.
Unfortunately that would allow passing a usize or similar, which really shouldn't be implicitly converted.
Since it's a private API, I think it's fine to allow this. If you want to make this more explicit, you could use Error::from_os_code::<isize>(code) at call sites. Either way, I do not insist on it.
There was a problem hiding this comment.
Well, I think the main goal here is to ensure that the result is well-defined, rather than to increase convenience.
I do hope this API can be improved further and then made public to replace the From<NonZeroU32> implementation as discussed elsewhere.
There was a problem hiding this comment.
I added the documentation, moved the ERRNO_NOT_POSITIVE logic back to where it belongs, and renamed the function to be more like the analogous std::io::Error function.
5391cbc to
52bcd6d
Compare
Avoid the `From<NonZeroU32>` implementation in favor of a constructor that centralizes all the range checking in one place. Consistently use `Self::UNEXPECTED` for out-of-rnage values. Besides being more consistent in the range checking, this also reduces the boilerplate in callers, which makes it easier to maintain the ports to less-common operating systems. This is a step towards deprecating the `impl From<NonZeroU32> for Error` implementation.
Avoid the `From<NonZeroU32>` implementation in favor of a constructor that centralizes all the range checking in one place. Besides being more consistent in the range checking, this also reduces the boilerplate in callers, which makes it easier to maintain the ports to less-common operating systems.
Avoid the
From<NonZeroU32>implementation in favor of a constructor that centralizes all the range checking in one place. Consistently useERRNO_NOT_POSITIVEfor nonpositive values andSelf::UNEXPECTEDfor too-large values.Besides being more consistent in the range checking, this also reduces the boilerplate in callers, which makes it easier to maintain the ports to less-common operating systems.