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.

Sign up for GitHub

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

Jump to bottom

slot-ctrl: remove two lines of unsafe #246

Merged
merged 1 commit into from
Nov 14, 2024
Merged

slot-ctrl: remove two lines of unsafe #246

merged 1 commit into from
Nov 14, 2024

Conversation

TheButlah
Copy link
Collaborator

No description provided.

@TheButlah TheButlah enabled auto-merge (squash) September 30, 2024 22:11
@TheButlah TheButlah force-pushed the ryanbutler-o-2877-make-orb-slot-ctrl-use-rustix branch from 1d178e0 to 8bb323d Compare September 30, 2024 22:14
fouge
fouge reviewed Oct 1, 2024
View reviewed changes
Copy link
Collaborator

@fouge fouge left a comment

Choose a reason for hiding this comment

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

is it really worth it? 🤔
it delegates the unsafe code to a third party crate but should we trust libc or rustix more?

@vmenge
Copy link
Contributor

vmenge commented Oct 1, 2024

is it really worth it? 🤔 it delegates the unsafe code to a third party crate but should we trust libc or rustix more?

Using libc like this causes issues on macOS :/

vmenge
vmenge approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@vmenge vmenge left a comment

Choose a reason for hiding this comment

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

LGTM! Did you test it on an orb?

@TheButlah
Copy link
Collaborator Author

is it really worth it?

Yes, long term I want to try to use rustix as much as possible.

I will eventually rewrite the Ioctl stuff using it. It provides good tools for Ioctl calls. See for example my PR to nusb: kevinmehall/nusb#65

@TheButlah
Copy link
Collaborator Author

Did you test it on an orb?

No, because the thing is the same function. But I can do that before merging.

@TheButlah TheButlah added the difficulty:trivial Can be merged if no review after 1 day label Oct 1, 2024
@fouge fouge self-requested a review November 12, 2024 09:10
@fouge
Copy link
Collaborator

fouge commented Nov 12, 2024

@TheButlah let's resolve the conflicts and merge

@TheButlah TheButlah force-pushed the ryanbutler-o-2877-make-orb-slot-ctrl-use-rustix branch from 8bb323d to 0953145 Compare November 14, 2024 20:35
@TheButlah TheButlah merged commit b3dbbe4 into main Nov 14, 2024
10 checks passed
@TheButlah TheButlah deleted the ryanbutler-o-2877-make-orb-slot-ctrl-use-rustix branch November 14, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vmenge vmenge vmenge approved these changes

@fouge fouge Awaiting requested review from fouge

Assignees
No one assigned
Labels
difficulty:trivial Can be merged if no review after 1 day
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TheButlah @vmenge @fouge