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

Enhance Guid::from_values and Guid::fmt #280

Merged
merged 5 commits into from
Sep 7, 2021
Merged

Enhance Guid::from_values and Guid::fmt #280

merged 5 commits into from
Sep 7, 2021

Conversation

timrobertsdev
Copy link
Contributor

PR for issue #279

@timrobertsdev
Copy link
Contributor Author

Text of issue #279

Hi, since const panics are now a thing, we can assert that the node id passed to Guid::from_values has the high 16 bits cleared and take a u64 instead of a [u8; 6]. I've also cleaned up the Display implementation for Guid, replacing the manual bitshifts with the u16::from_be_bytes and u64::from_be_bytes stdlib functions. The functionality is identical (it actually ends up compiling down to the same thing, according to godbolt), but the code ends up being simpler and easier to read.

The included test suite passes with either or both changes.

src/table/runtime.rs Outdated Show resolved Hide resolved
src/table/cfg.rs Outdated Show resolved Hide resolved
@nicholasbishop
Copy link
Contributor

nicholasbishop commented Sep 7, 2021

I think currently there aren't any tests that explicitly check the functioning of the Guid code, although it's probably done implicitly by some of the other tests. To make sure the changes here behave as expected it would be good to add some unit tests to src/data_types/guid.rs, something like this:

#[cfg(test)]
mod tests {
    extern crate alloc;
    use super::*;

    #[test]
    fn test_guid_display() {
        assert_eq!(
            alloc::format!(
                "{}",
                Guid::from_values(0x12345678, 0x9abc, 0xdef0, 0x1234, 0x56789abcdef0)
            ),
            "12345678-9abc-def0-1234-56789abcdef0"
        );
    }

    #[test]
    fn test_unsafe_guid() {
        #[unsafe_guid("12345678-9abc-def0-1234-56789abcdef0")]
        struct X;

        assert_eq!(
            X::GUID,
            Guid::from_values(0x12345678, 0x9abc, 0xdef0, 0x1234, 0x56789abcdef0)
        );
    }
}

@timrobertsdev
Copy link
Contributor Author

Got the tests added as well, they are both passing. Actually getting cargo test to run was interesting, I ended up using cargo -Z panic-abort-tests -Z build-std=panic_abort,std test --target x86_64-unknown-linux-gnu. Is there a better way to run tests for no_std projects?

I noticed that all of the doctests were either ignored or failed compilation, I'm assuming from us either being in a panic=abort environment. Unit tests are okay though, once the proper cargo options are set.

@GabrielMajeri
Copy link
Collaborator

Is there a better way to run tests for no_std projects?

The command used in CI is something like CARGO_PROFILE_DEV_PANIC=unwind cargo test -Z build-std=std test --target x86_64-unknown-linux-gnu. It should make all the doc tests to pass correctly.

I've opened #281 to look into better documenting how to run the doc/unit tests.

@GabrielMajeri GabrielMajeri changed the title Enhance Guid::from_values and Guid::fmt Enhance Guid::from_values and Guid::fmt Sep 7, 2021
@GabrielMajeri GabrielMajeri merged commit 400c905 into rust-osdev:master Sep 7, 2021
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.

3 participants