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

Use Rust union types #2056

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

Use Rust union types #2056

wants to merge 4 commits into from

Conversation

kellda
Copy link
Contributor

@kellda kellda commented Feb 4, 2021

For #1020

Replace some types with unions now that rustc supports them.

I also found sigevent.sigev_notify_thread_id, siginfo_t.__data and sigaction.sa_sigaction that I think should be unions, but I wasn't sure.

@rust-highfive
Copy link

r? @Amanieu

(rust-highfive has picked a reviewer for you, use r? to override)

@kellda
Copy link
Contributor Author

kellda commented Feb 4, 2021

Also I had to ignore iffaddrs.ifa_ifu on Linux to pass CI

@Amanieu
Copy link
Member

Amanieu commented Feb 4, 2021

Unfortunately I believe this is a breaking change, which means we can't actually do this until we bump the crate version to 1.0.

cc @JohnTitor

@kellda
Copy link
Contributor Author

kellda commented Feb 4, 2021

I'm aware that it is a breaking change and that's not a problem for me to wait until a new major version for this to be merged.

@bors
Copy link
Contributor

bors commented Feb 13, 2021

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

@tgross35
Copy link
Contributor

tgross35 commented Aug 29, 2024

@kellda main is now intended for libc 1.0. Are you able to rebase this?

@rustbot author

@rustbot
Copy link
Collaborator

rustbot commented Aug 29, 2024

Some changes occurred in solarish module

cc @jclulow, @pfmooney

@kellda
Copy link
Contributor Author

kellda commented Aug 29, 2024

I'm unsure how to find out what's wrong with sigval on emscripten...

@kellda
Copy link
Contributor Author

kellda commented Aug 29, 2024

Btw, I noted (when I originally opened this PR) that the following fields may also be wroth considering turning into unions:

  • sigevent.sigev_notify_thread_id
  • siginfo_t.{_pad/__data}
  • sigaction.sa_sigaction

@tgross35
Copy link
Contributor

tgross35 commented Oct 1, 2024

I'm unsure how to find out what's wrong with sigval on emscripten...

I also don't know. I'll mark this as review as a reminder that I need to take a look at this.

@rustbot review

Btw, I noted (when I originally opened this PR) that the following fields may also be wroth considering turning into unions:

* sigevent.sigev_notify_thread_id

* siginfo_t.{_pad/__data}

* sigaction.sa_sigaction

Feel free to submit another PR!

@tgross35
Copy link
Contributor

This needs another rebase, feel free to just leave emscripten disabled in build.rs as a FIXME if needed.

Regarding traits: don't add Hash/PartialEq/Eq for unions, or leave them but make the bodies unimplemented!("traits") if some other structs depend on them. We are likely going to drop these in 1.0 so we aren't doing any unsafe union field access.

For Debug, make the unions fmt.debug_struct("type_name").finish_non_exhaustive().

@rustbot author

@rustbot rustbot added the O-unix label Nov 21, 2024
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.

7 participants