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

ByteOrder supertraits are a compatibility hazard #69

Closed
dtolnay opened this issue Feb 25, 2017 · 1 comment
Closed

ByteOrder supertraits are a compatibility hazard #69

dtolnay opened this issue Feb 25, 2017 · 1 comment
Labels

Comments

@dtolnay
Copy link

@dtolnay dtolnay commented Feb 25, 2017

pub trait ByteOrder
    : Clone + Copy + Debug + Default + Eq + Hash + Ord + PartialEq + PartialOrd {

I understand why these exist (#60) but I am worried about the situation if Rust were to add another trait with a builtin derive not on this list. Our options would be to add it to this list which would require a 2.0 release of byteorder, or not add it to this list which means the new derive won't work nicely with structs parameterized over byteorder.

I think we should take steps to prevent extending this list being a breaking change. In serde_json we use a trick like this in a similar situation:

mod private {
    pub trait Sealed {}
    impl Sealed for super::LittleEndian {}
    impl Sealed for super::BigEndian {}
}

/// This trait is sealed and cannot be implemented for types outside of byteorder.
pub trait ByteOrder
    : Clone + Copy + Debug + ... + private::Sealed {

Now we are free to add supertraits and even trait methods (u128 i128 #65) without a major release.

The selling point of byteorder is that you can read and write little- / big-endian numbers, not that you can define your own wild and crazy byte orders, so I think it is reasonable to limit implementations to within the byteorder crate.

cc @fitzgen

@brson brson mentioned this issue Mar 4, 2017
7 of 7 tasks complete
@BurntSushi
Copy link
Owner

@BurntSushi BurntSushi commented Mar 28, 2017

I think we should do this. Thank you for the idea. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.