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.

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

aipc, an async ipc library for aero #60

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pitust
Copy link
Contributor

@pitust pitust commented Mar 19, 2022

'nuff said.

@pitust pitust changed the title Introducing aipc, an async ipc library for aero aipc, an async ipc library for aero Mar 19, 2022
@48cf
Copy link
Collaborator

48cf commented Mar 19, 2022

I'm not a fan of all these println!s polluting the stdout of the running process, even if they are unlikely to happen in most cases. You should return Results instead of printing the error and carrying on/signaling failure upstream. Also there is quite a few unwraps that could be turned into Results too.

@pitust
Copy link
Contributor Author

pitust commented Mar 19, 2022

I'm not a fan of all these println!s polluting the stdout of the running process, even if they are unlikely to happen in most cases. You should return Results instead of printing the error and carrying on/signaling failure upstream. Also there is quite a few unwraps that could be turned into Results too.

I could possibly move them to sys_log, as not to pollute stdout. The majority of the errors shouldn't propagate above AsyncRuntime::run() anyway.

The unwraps are there because I explicitly wanted to prevent injecting Result, but if you have an idea how to make it work without destroying the ergonomics, please tell me!



fn main() {
let mut rt = aipc::async_runtime::AsyncRuntime::new();

Choose a reason for hiding this comment

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

Instead of constructing a runtime manually at the start of the main function, it would be a much cleaner idea to initialize it lazily in the spawn function instead. So it will look like apic::spawn(async {}). In future we; after we get the rust userland port sorted out we can switch to using tokio as our async runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants