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

Remove extra traits #3692

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Remove extra traits #3692

wants to merge 3 commits into from

Conversation

rj00a
Copy link

@rj00a rj00a commented May 10, 2024

This PR removes the extra_traits feature and all implementations of Eq, Hash, and Debug as mentioned in #3248.

The decision to remove Debug impls was left as an open question, but I've gone ahead and removed them here in the interest of maintainability and consistency.

Closes #2700, #2064, #2816

@rustbot
Copy link
Collaborator

rustbot commented May 10, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot
Copy link
Collaborator

rustbot commented May 10, 2024

Some changes occurred in OpenBSD module

cc @semarie

Some changes occurred in solarish module

cc @jclulow, @pfmooney

Some changes occurred in OpenBSD module

cc @semarie

@@ -140,9 +139,6 @@ fn check_style(file: &str, path: &Path, err: &mut Errors) {
} else if line.starts_with("type ") && !in_impl {
State::Typedefs
} else if line.starts_with("s! {") {
s_macros += 1;
Copy link
Author

Choose a reason for hiding this comment

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

I had to allow multiple s! macros in one module because the s_no_extra_traits! macro no longer exists.

@bors
Copy link
Contributor

bors commented Jun 25, 2024

☔ The latest upstream changes (presumably #3617) made this pull request unmergeable. Please resolve the merge conflicts.

@myl7
Copy link

myl7 commented Aug 5, 2024

Hi, I am the one who is working on the project to modernize the libc crate (Related: OSPP Idea OSPP Zulip).
I would like to ask that if I could rebase your commits and develop upon them, to complete the "trait implementations" change for libc v1?

@SteveLauC
Copy link
Contributor

I would like to ask that if I could rebase your commits and develop upon them, to complete the "trait implementations" change for libc v1?

Hi, would you like to show me what these trait impl would look like in libc 1.0, this is not covered in the tracking issue and that OSPP page. Nix, as a big consumer of the libc crate, uses them a lot: nix-rust/nix#2468

@erickt
Copy link
Contributor

erickt commented Aug 7, 2024

As an alternative to getting rid of extra_traits, another direction could be to use or build a library like zerocopy to assert at compile time that all the fields of a union can be referenced without UB? That could be done through a test-only cfg to reduce compile times, or if the MSRV didn't support this or something.

@tgross35 tgross35 added this to the 1.0 milestone Aug 13, 2024
@myl7
Copy link

myl7 commented Aug 17, 2024

what these trait impl would look like in libc 1.0

According to the "Trait implementations" section, I think Eq and Hash impl would be completely removed.
Debug impl may be kept, but at least future code would not impl it.

@tgross35
Copy link
Contributor

Hey @myl7: we still need to figure out what exactly we want to do with traits here (at this point I'm thinking we should keep Debug but remove the rest, but that hasn't been discussed), but any chance you would want to pick up #4118? That is something we could merge today, and would make the diff here a bit smaller once we have a final decision.

@tgross35
Copy link
Contributor

@rj00a are you able to finish this up? If so, please drop PartialEq, Eq, and Hash but leave Debug.

@rustbot author

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

Successfully merging this pull request may close these issues.

Broken PartialEq impl for sigset_t on Linux
8 participants