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

Implement more traits for guard types #44780

Closed

Conversation

marcusbuffett
Copy link
Contributor

Implements Hash, PartialEq, Eq, PartialOrd, and Ord for
Ref, RefMut, MutexGuard, RwLockReadGuard, RwLockWriteGuard.

Fixes #24372

Bit of a rust noob, quick questions:

Preferred style wrt x.deref() or *x? I went with * unless it was going to be something too weird like &**.

hash::Hash or Hash? I see that fmt::Display and fmt::Debug are used for the Display and Debug traits, should I prefer that syntax instead?

I saw all the other implementations use #[inline] and I just followed suit, any rules of thumb here, or is it just "use inline everywhere always?"

I didn't see anything in the issue that said exactly what types needed these traits, so I just went with all the types that got Debug and Display in this commit: 14df549, any more types that need these traits?

Implements Hash, PartialEq, Eq, PartialOrd, and Ord for
Ref, RefMut, MutexGuard, RwLockReadGuard, RwLockWriteGuard

Fixes rust-lang#24372
@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 @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.

}
}

#[stable(feature = "std_guard_impls_ext", since = "1.23.0")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe these should be marked as unstable with a tracking issue set, but I'll let anyone else in the team correct me.

Copy link
Member

Choose a reason for hiding this comment

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

We don't track stability on impls of traits today so these are all insta-stable AFAIK; no need to change anything.

@estebank estebank added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 23, 2017
}
}

#[stable(feature = "std_guard_impls_ext", since = "1.23.0")]
Copy link
Member

Choose a reason for hiding this comment

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

Unless you expect this PR will stay open for 20 days, the since field should still be 1.22.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

@BurntSushi
Copy link
Member

This all seems fine to me, but I'm kind of surprised we didn't already have these impls. @marcusbuffett Could you maybe elaborate on the use case for wanting each of these impls for these types?

cc @rust-lang/libs Is there a reason why we don't have these impls already or is it just that nobody has added them yet?

@BurntSushi BurntSushi added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2017
@marcusbuffett
Copy link
Contributor Author

@BurntSushi Beyond pointing to the issue (#24372), I can't really elaborate much – I just searched for E-easy && E-Help Wanted tags and chose one. I don't know enough yet about the types to say why the impls should or shouldn't exist. I'd just be guessing, and your guess is better than mine.

@alexcrichton
Copy link
Member

Seems like a find idea to me! I think we just haven't gotten around to this previously

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Seems okay to me. Thanks!

@arielb1 arielb1 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 26, 2017
@alexcrichton
Copy link
Member

Ok we got a chance to discuss this during the libs triage meeting today. In discussing this we ended up being increasingly wary of the guideline we set forth to add these trait impls and ended up concluding that we may not wish to add these at this time.

These implementations, as added here, wouldn't enable something like MutexGuard<i32> == i32, but that's arguably something we'd like to work. Adding that, however, precludes MutexGuard<i32> == MutexGuard<i32>. We didn't believe there was a clear way to implement this right now as all current strategies had downsides.

The possibility of specialization in the future may end up providing a solution to this problem, and @aturon also mentioned that the lang team is going to be working during the impl period on "deref coercion ergonomics" to alleviate much of the need for these impls in the first place.

In light of all that I'm going to close this for now and we'll likely be removing the guideline indicating that these traits should be implemented temporarily. Regardless though thanks for the PR @marcusbuffett!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants