-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
move personality to sys #108796
move personality to sys #108796
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the only problem here is that this now results in a bunch of cfg being done separately in the "platform" file and also this personality file. I am half-inclined to say we should approve this anyway, since that was a preexisting condition and it really helps to have "move these files" as a separate commit in git.
However, I would still prefer a followup commit that restructures the personality code so that at least the cfg selection of which personality implementation is inside the "platform file". You don't have to do anything on top of that, just thinning out the cfgoo from personality/mod.rs
☔ The latest upstream changes (presumably #108934) made this pull request unmergeable. Please resolve the merge conflicts. |
832f7e8
to
9b17591
Compare
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
r? workingjubilee |
☔ The latest upstream changes (presumably #109346) made this pull request unmergeable. Please resolve the merge conflicts. |
@devsnek You need to rebase on master rather than having a merge commit. Also would you mind adding a commit that addresses @workingjubilee's concerns, specifically:
Feel free to ask for clarification if needed. |
☔ The latest upstream changes (presumably #110214) made this pull request unmergeable. Please resolve the merge conflicts. |
@workingjubilee i'm not entirely sure what code you want moved where. |
Hm, I'm not entirely sure I can do a good job explaining it, but it's mostly about how ideally, the "top level" cfg for sys... the one in library/std/src/sys/mod.rs... is the thing that picks which implementation of things to use and for the rest it's hypothetically "small" differences (but we've both seen inside library/std/src/sys/unix/ so we both know that's a damn lie... but it's nice when it's truer). If you still feel uncertain about what I mean then just rebase this and I'll approve it and figure out the followup on my own time, I have an idea for how things should be restructured in sys in general based on some recent forays. |
65f656f
to
90e11a2
Compare
I took care of the rebase. |
☀️ Test successful - checks-actions |
1 similar comment
☀️ Test successful - checks-actions |
Finished benchmarking commit (743333f): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 658.446s -> 656.93s (-0.23%) |
…=workingjubilee move personality to sys this moves `personality` to sys, removing another PAL exception
this moves
personality
to sys, removing another PAL exception