Conversation
A6GibKm
force-pushed
the
safe-io
branch
2 times, most recently
from
1200fdd to
281adac
Compare
|
Added commit descriptions where there were api questions. |
A6GibKm
force-pushed
the
safe-io
branch
4 times, most recently
from
2b2aeee to
b8c5eb6
Compare
|
|
A6GibKm
force-pushed
the
safe-io
branch
2 times, most recently
from
16e67be to
86cfed9
Compare
|
Rebased. There were quite many conflicts so a review would be appreciated. |
|
Is this ready for another review and potentially merging? |
Yes and maybe |
| stderr_fd: V, | ||
| stdin_fd: Option<impl AsFd>, | ||
| stdout_fd: Option<impl AsFd>, | ||
| stderr_fd: Option<impl AsFd>, |
There was a problem hiding this comment.
Option<impl ...> is usually a bad idea. Just try calling this function with a None :)
I think here and in the other APIs using an Option<impl ...> currently you'll have to add a builder pattern instead
| #[cfg(unix)] | ||
| #[doc(alias = "g_unix_open_pipe")] | ||
| pub fn unix_open_pipe(flags: i32) -> Result<(RawFd, RawFd), Error> { | ||
| pub unsafe fn unix_open_pipe(flags: i32) -> Result<(RawFd, RawFd), Error> { |
There was a problem hiding this comment.
Is that compatible with the flags argument that allows setting O_CLOEXEC/FD_CLOEXEC?
There was a problem hiding this comment.
As a prior art rustix::fd::open also returns OwnedFd and allows passing in the CLOEXEC flag.
| #[cfg_attr(docsrs, doc(cfg(not(windows))))] | ||
| #[doc(alias = "g_spawn_async_with_pipes")] | ||
| pub fn spawn_async_with_pipes< | ||
| pub unsafe fn spawn_async_with_pipes< |
There was a problem hiding this comment.
Why not make this use the new traits?
| /// The default main loop almost always is the main loop of the main thread. | ||
| /// Thus, the closure is called on the main thread. | ||
| #[doc(alias = "g_unix_fd_add_full")] | ||
| pub fn unix_fd_add<F>(fd: RawFd, condition: IOCondition, func: F) -> SourceId |
There was a problem hiding this comment.
I think these functions all need to be unsafe actually. impl AsFd is correct but it's up to the caller to ensure that the fd is valid long enough
|
Perhaps we could split this into smaller chunks. |
|
Go for it, or maybe @nagisa wants to take this over to bring it over the finish line? |
|
@A6GibKm would you mind rebasing this one? i would like to switch to 2024 edition before the next breaking release |
|
How does this relate to 2024 edition? But regardless, it would be nice to finish this :) |
|
Rebased. |
|
Sure, go ahead :) |
|
I am closing in favor of #1745. If someone wants to pick up wip: unix_fd_list: Use custom iterator for steal_fds, feel free to do so. |
|
Can you create an issue for tracking the missing parts? |
4 participants
Needs reviewing
Stuff thats missing