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

Deprecate and remove impl From<NonZeroU32> for Error. #455

Closed
briansmith opened this issue Jun 4, 2024 · 6 comments · Fixed by #507
Closed

Deprecate and remove impl From<NonZeroU32> for Error. #455

briansmith opened this issue Jun 4, 2024 · 6 comments · Fixed by #507
Labels
enhancement New feature or request
Milestone

Comments

@briansmith
Copy link
Contributor

The From<NonZeroU32> for Error implementation doesn't do any checking of its inputs; for example, it doesn't verify that an "internal" error code isn't being used, nor does it verify that any internal error code is actually a valid/known one.

I propose:

This way, internal errors will eventually only be able to be constructed from within the crate.

@briansmith briansmith mentioned this issue Jun 4, 2024
@josephlr josephlr added this to the Next Release milestone Jun 4, 2024
@josephlr
Copy link
Member

josephlr commented Jun 21, 2024

I like this idea, having something like:

impl Error {
  pub const fn from_os_error(i32) -> Self;
  pub const fn raw_os_error(self) -> Option<i32>

  pub const fn new_custom(u16) -> Self;
  const fn new_internal(u16) -> Self;
}

seems reasonable to me. We may want to use i32 consistently for OS errors as that's what libstd uses (and it's the return type of errno).

EDIT: Should we also remove the Error::code method?

@dhardy
Copy link
Member

dhardy commented Nov 29, 2024

EDIT: Should we also remove the Error::code method?

@newpavlov decided to do this in #507, but I don't see a replacement. Should we add the following?

pub const fn custom_code(self) -> Option<u16>;

@dhardy dhardy reopened this Nov 29, 2024
@newpavlov
Copy link
Member

newpavlov commented Nov 29, 2024

@dhardy
What is the use-case for it I believe it should be sufficient to match Error with the associated constants. The absence of such method allows us to make the custom codes "private" and change them if necessary.

@dhardy
Copy link
Member

dhardy commented Nov 29, 2024

If a custom source is used and it is wished to handle these errors somehow later — but no, I don't have any specific use case in mind.

The absence of such method allows us to make the custom codes "private" and change them if necessary.

Nothing changes here since we already have pub const fn new_custom and custom_code can use internal arithmetic.

@newpavlov
Copy link
Member

newpavlov commented Nov 29, 2024

I meant that we can change for example UNSUPPORTED from new_internal(0) to new_internal(42).

Addition of such method will be a backwards compatible change, so we can do it later in the case if someone requests such method.

@dhardy
Copy link
Member

dhardy commented Nov 29, 2024

Changing CUSTOM_START would be a non-breaking change either way (ignoring the Debug output).

Sure, this can be added later if needed.

@dhardy dhardy closed this as completed Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants