-
Notifications
You must be signed in to change notification settings - Fork 5
Add CXX child crate #18
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
base: main
Are you sure you want to change the base?
Conversation
|
Is the plan for dcsctp to be an alternative to https://github.com/sctplab/usrsctp or https://github.com/webrtc-rs/sctp-proto |
e365ee9 to
c1a35c5
Compare
This adds CXX FFI for the dcSCTP rust library to integrate it into a C++ application.
To avoid polluting the default namespace.
| @@ -0,0 +1 @@ | |||
| /examples/main | |||
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.
Could you move this to the top level .gitignore instead? Makes it easier to maintain.
|
|
||
| fn delete_socket(socket: *mut DcSctpSocket) { | ||
| if !socket.is_null() { | ||
| unsafe { |
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.
Add a // SAFETY: ... comment explaining why this is safe
| if !socket.is_null() { | ||
| unsafe { | ||
| // Re-take ownership of the Box and drop it. | ||
| let _ = Box::from_raw(socket); |
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.
Could you do this instead?
drop(Box::from_raw(socket));
| #[derive(Debug)] | ||
| enum SocketState { | ||
| Closed, | ||
| Connecting, | ||
| Connected, | ||
| ShuttingDown, | ||
| } | ||
|
|
||
| extern "Rust" { | ||
| type DcSctpSocket; |
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.
Is it not possible to re-export dcsctp::api::SocketState here? Why do you need to create a duplicate inside the ffi module?
...but in that case I think you would need to return Box<SocketState> from state().
| } | ||
| } | ||
|
|
||
| unsafe fn state(socket: *const DcSctpSocket) -> ffi::SocketState { |
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.
Can you take a &DcSctpSocket instead of *const DcSctpSocket?
| assert!(!socket.is_null()); | ||
| &*socket | ||
| }; | ||
| match socket.0.state() { |
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.
Is this part unsafe? Do you need the whole function block to be unsafe or can you limit it?
| } | ||
| } | ||
|
|
||
| #[allow(dead_code)] |
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.
Is this needed? It's pub so why would it complain about being unused? Does this have to be pub by the way?
It currently has a single function, to query the library's version, to validate that the C++ FFI is working correctly.