Skip to content

Revise error types and codes #11

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

Merged
merged 9 commits into from
Mar 14, 2019
Merged

Revise error types and codes #11

merged 9 commits into from
Mar 14, 2019

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Mar 1, 2019

This makes a few changes:

  • revises error codes to have a common prefix
  • allows custom codes and messages per target
  • makes the error module public

It feels quite clunky having to add an empty fn error_msg_inner to all modules, but I don't see a better approach.

@newpavlov review? I think this concludes all my to-do list for the first release.

BTW the dummy module was broken; we don't appear to have a test for it (I hacked lib.rs locally).

dhardy added 2 commits March 1, 2019 11:27
Versions found by examining API or trial-and-error.
CI will now try minimal lib versions with minimal rustc
version, but only for select targets.
dhardy added 2 commits March 1, 2019 12:00
Option isn't compatible with stable Rustc.
Nightly runners already have important tests for latest version.
Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me! Though I think we do not need error module and we can re-export Error type to the crate root.

Also looks there is a typo in Error::code docs:

This may equal one of the codes defined in this library or may be a system error code.

Fix to "this may be equal to one of <..>"? Also shouldn't "it" or "the return code" be used here instead of "this"?

// A randomly-chosen 16-bit prefix for our codes
pub(crate) const CODE_PREFIX: u32 = 0x57f40000;
const CODE_UNKNOWN: u32 = CODE_PREFIX | 0;
const CODE_UNAVAILABLE: u32 = CODE_PREFIX | 1;
Copy link
Member

@newpavlov newpavlov Mar 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should make UNKNOWN and UNAVAILABLE associated constants of Error type?

@dhardy
Copy link
Member Author

dhardy commented Mar 4, 2019

We also don't need the std::error module, but I don't think it gets in the way? At least in this case the other error types can have better names.

@newpavlov
Copy link
Member

I think a better analogy will be std::io::error, which as you know does not exist. Think in terms of potential inclusion into std.

@dhardy
Copy link
Member Author

dhardy commented Mar 5, 2019

And yet std::io::prelude exists, while there are several error-related things in std::io. But okay, I guess we can do away with the error module.

@newpavlov
Copy link
Member

newpavlov commented Mar 7, 2019

BTW maybe it's worth to add UNKNOWN and UNAVAILABLE error code values to relevant constant docstrings (in both hex and dec forms), so it will be possible to search for them via search engines?

@newpavlov
Copy link
Member

newpavlov commented Mar 11, 2019

Sorry, I meant "Error(0x{:08X})" in the comment earlier. Also what do you think about associated constants? And maybe we should use 24-bit prefix for easier search? For example Ox57f4c5OO results in zero google hits.

@dhardy
Copy link
Member Author

dhardy commented Mar 12, 2019

Okay. I added the mentioned logging. What do you think about this? Many of the messages are not hugely useful, but this feature is optional and it's still not much overhead.

@newpavlov
Copy link
Member

newpavlov commented Mar 12, 2019

No objections, though I am also not sure about usefulness, as you either get a panic, or process error explicitly.

@dhardy
Copy link
Member Author

dhardy commented Mar 13, 2019

Agreed. Still, I don't wish to spend the time on trivial tweaks now so this will do (once I fix tests).

@dhardy
Copy link
Member Author

dhardy commented Mar 14, 2019

FYI, libc version bumped to solve this error. I lack motivation to find the minimal version required.

@newpavlov
Copy link
Member

I can prepare a PR with some minor tweaks after you'll merge this one.

@dhardy dhardy merged commit 170cbc1 into rust-random:master Mar 14, 2019
@dhardy
Copy link
Member Author

dhardy commented Mar 14, 2019

Please do.

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

Successfully merging this pull request may close these issues.

2 participants