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

[libcore/cmp] Expand Ord/PartialOrd Derivable doc for enum types #42920

Merged
merged 1 commit into from
Jun 29, 2017

Conversation

behnam
Copy link
Contributor

@behnam behnam commented Jun 27, 2017

Expand Derivable docblock section for Ord and PartialOrd to cover
enum types, in addition to the existing language explaining it for
struct types.

Expand Derivable docblock section for `Ord` and `PartialOrd` to cover
`enum` types, in addition to the existing language explaining it for
`struct` types.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@behnam
Copy link
Contributor Author

behnam commented Jun 27, 2017

Context: compare with Derivable section of PartialEq, which explains behavior for struct and enum separately: https://doc.rust-lang.org/std/cmp/trait.PartialEq.html#derivable. The language added in this diff is based on that.

@behnam
Copy link
Contributor Author

behnam commented Jun 27, 2017

Something slightly related here: at the moment, looking at the library docs and the book, it's not clear how PartialEq can be implemented for enum values, specially when they are not Eq.

For example, let's say we have

enum BookFormat {
    Paperback,
    Hardback,
    Ebook,
    Unknown,
}

where we expect Unknown != Unknown.

Then, in this case, the eq() implementation needs to use match self to detect Unknown value, and trying to use == results in unconditional recursion.

Do you think this is something that can be address in libcore/cmp.rs?

@arielb1 arielb1 assigned BurntSushi and unassigned brson Jun 27, 2017
@arielb1
Copy link
Contributor

arielb1 commented Jun 27, 2017

r? @BurntSushi

@arielb1 arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 27, 2017
@BurntSushi
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 27, 2017

📌 Commit 330dab8 has been approved by BurntSushi

@BurntSushi
Copy link
Member

@behnam Seems like you probably just want to implement PartialEq explicitly? (If I'm misunderstanding, maybe open a separate issue?)

@behnam
Copy link
Contributor Author

behnam commented Jun 28, 2017

Thanks, @BurntSushi. Yeah, let me make a good example and file a separate issue.

@arielb1
Copy link
Contributor

arielb1 commented Jun 29, 2017

@bors rollup

arielb1 pushed a commit to arielb1/rust that referenced this pull request Jun 29, 2017
[libcore/cmp] Expand Ord/PartialOrd Derivable doc for enum types

Expand Derivable docblock section for `Ord` and `PartialOrd` to cover
`enum` types, in addition to the existing language explaining it for
`struct` types.
bors added a commit that referenced this pull request Jun 29, 2017
Rollup of 12 pull requests

- Successful merges: #42219, #42831, #42832, #42884, #42886, #42901, #42919, #42920, #42946, #42953, #42955, #42958
- Failed merges:
@bors bors merged commit 330dab8 into rust-lang:master Jun 29, 2017
@behnam behnam deleted the cmp branch June 29, 2017 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants