Derive std::cmp::Reverse as Copy or Clone

[da-x]

If the type parameter is Copy or Clone, then Reverse should be too.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (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.

[Mark-Simulacrum]

Shouldn't we add Hash and Default for this?

[shepmaster]

@Mark-Simulacrum can you expand on what you mean by "for this"? Do you mean "since we are adding the Copy and Clone traits, we should also add Hash and Default?

[Mark-Simulacrum]

Yes, that's what I meant. Sorry for not being clear.

[da-x]

Interesting. If there is any unwritten 'gold standard' for wrapper type deriving in libcore, then perhaps it is apparent by looking at Option, Result, and others. Perhaps we should match ourselves to them, i.e:

#[derive(Clone,Copy,PartialEq,Eq,Debug,Hash)]// PartialOrd and Ord specialized here

What do you think?

EDIT: PartialOrd and Ord are specialized here.

[Mark-Simulacrum]

Option also implements Default, which we should derive as well; Option needs to implement it as an enum but this struct can derive it.

[shepmaster]

Actually, iterators shouldn't implement Copy. That's how it used to be before Rust 1.0, but that behavior was deemed too confusing and was removed. The explicit clone call is preferred.

[eddyb]

@shepmaster I wish we had a canonical thing to point to - but it's basically blocked on a lint.

[goffrie]

Actually, iterators shouldn't implement Copy

Reverse isn't an iterator though ;)

[kennytm]

Review ping for you @BurntSushi!

[shepmaster]

Reverse isn't an iterator though ;)

Haha, I was thinking std::iter::Rev, not std::cmp::Reverse, oops!

[BurntSushi]

It seems like we probably want a Hash impl on this too. But this LGTM.

@rfcbot fcp merge

[kennytm]

@BurntSushi I guess rfcbot doesn't recognize edits.

[BurntSushi]

@rfcbot fcp merge

[rfcbot]

Team member @BurntSushi has proposed to merge this. The next step is review by the rest of the tagged teams:

• @BurntSushi
• @Kimundi
• @KodrAus
• @alexcrichton
• @aturon
• @dtolnay
• @sfackler
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[shepmaster]

Ping for ticky boxes, @Kimundi !

[emilyalbini]

@Kimundi there is a nice checkbox in #47379 (comment) waiting for you!

[da-x]

Hey @Kimundi can you please respond?

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[Kimundi]

Sorry! I'm really bad at staying on top of my github notifications, and I thought I had already checked this one at the last ping...

[sfackler]

@bors r+

[bors]

📌 Commit 96157ef has been approved by sfackler

[kennytm]

@bors rollup