Fixing the improper type of sighandler_t on unix#4918
Fixing the improper type of sighandler_t on unix#4918ItshMoh wants to merge 5 commits intorust-lang:mainfrom
Conversation
|
hey @tgross35 I have continued the PR #2107 , I have removed the custom Debug implementation since one already exists in macros.rs here. Could you please take a look at the PR and let me know if this approach looks good? |
|
@ItshMoh you need to run Also, please rebase to squash your commits, making sure you include the exact string below for correct attribution:
|
|
Reminder, once the PR becomes ready for a review, use |
| pub sa_handler: Option<extern "C" fn(c_int) -> ()>, | ||
| pub sa_sigaction: Option<extern "C" fn( |
There was a problem hiding this comment.
Could you make these functions unsafe?
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Please delete these, we are avoiding adding more iffy trait impls (even though this one is reasonably sound)
| pub type locale_t = *mut c_void; | ||
|
|
||
| s_no_extra_traits! { | ||
| pub union __c_anonymous_sigaction_handler{ |
There was a problem hiding this comment.
This doesn't need to be a __c_anonymous type, it can just be union sighandler_t with no alias since we're exporting it this way anyway.
| pub type locale_t = *mut c_void; | ||
|
|
||
| s_no_extra_traits! { | ||
| pub union __c_anonymous_sigaction_handler{ |
There was a problem hiding this comment.
Could you also add a comment about what the variants are representing? The two function pointers come from source, the integer variant is needed since sentinel values that aren't function pointers are used.
(maybe there's a better name for default? I think raw may make more sense)
|
I'm sort of thinking that maybe we should have a type indicating raw function pointers and have those in a two-variant union, rather than a three-variant union with Rust function pointers. Sketched some of this up at #4998, curious what you think @JohnTitor |
CoAuthored by : Bradley Landherr <https://github.com/landhb>
CoAuthored by : Bradley Landherr <https://github.com/landhb>
CoAuthored by : Bradley Landherr <https://github.com/landhb>
CoAuthored by : Bradley Landherr <https://github.com/landhb>
…dler as it is already implemented.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
5 participants
Fixes: #1359
Continued from #2107