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

Move to non-enum Status for type safety reasons #51

Merged
merged 17 commits into from
Sep 27, 2018
Merged

Conversation

HadrienG2
Copy link
Contributor

@HadrienG2 HadrienG2 commented Sep 26, 2018

As discussed in #48 and #44 before that, it is not safe to use a Rust enum for UEFI status codes due to their open-ended nature. For this reason, this PR proposes to use a newtype of usize and a bunch of constants instead.

A small amount of macromancy is used to reduce the noise and repetition that is inherent to this approach and keep the code readable.

Fixes #48.

src/error/status.rs Outdated Show resolved Hide resolved
src/error/status.rs Show resolved Hide resolved
src/table/boot.rs Outdated Show resolved Hide resolved
src/error/status.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@GabrielMajeri GabrielMajeri left a comment

Choose a reason for hiding this comment

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

Instead of making them constaints in the status module, what if we just made them associated constants of the Status type?

pub struct Status ...;

impl Status {
    pub const SOME_ERROR: Self = 0;
}

This should greatly reduce the number of changes required.

@HadrienG2
Copy link
Contributor Author

@GabrielMajeri Cool, did not now you could use that outside of the trait system! Implemented. Now there's only the minor warning question to be resolved and we should be all set.

@GabrielMajeri
Copy link
Collaborator

(CI failed due to rustfmt)

@HadrienG2
Copy link
Contributor Author

HadrienG2 commented Sep 27, 2018

In principle, one could build a macro which would reproduce the full enum feeling by using recursion to generate a sequence of numbers from a starting point. However...

  • I'm not sure what the impact of all that recursion would be on compile times.
  • That would add a fair bit of complexity which I would be relucant to add just for our status codes. I would feel less uneasy if we had other C-like enums to deal with in a similar way (maybe we do!), or if this were available as part of a more general FFI helper crate.

@GabrielMajeri
Copy link
Collaborator

@HadrienG2 I don't expect to see a lot of status codes being added to the spec anytime soon. The current solution is good.

@HadrienG2 HadrienG2 merged commit 9bce39c into master Sep 27, 2018
@HadrienG2 HadrienG2 deleted the safe-status branch September 27, 2018 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants