-
Notifications
You must be signed in to change notification settings - Fork 537
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
Replacing panics with Result
returns, and adds platform-specific status codes
#4690
Conversation
} | ||
Ok(()) |
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.
- Does Rust allow for non-zero
Ok
status codes? For instance, we have the 'pending' status. - Should we return
Ok(status)
here?
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.
Yes, Rust allows to return Ok(status), so we can inform whether the operation is complete or pending.
But I'm not sure that complete/pending information is useful.
AFAIK, msquic doesn't provide the function to wait when pending returns. So, I consider that we don't have to return complete/pending information.
src/lib.rs
Outdated
@@ -1782,8 +1915,8 @@ extern "C" fn test_stream_callback( | |||
|
|||
#[test] | |||
fn test_module() { | |||
let api = Api::new(); | |||
let registration = Registration::new(&api, ptr::null()); | |||
let api = Api::new().unwrap(); |
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.
If this returns an Err
will unwrap
trigger the panic now?
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.
Yes. If we want to avoid panic in test, we can write as below:
let api = match Api::new() {
Ok(api) => api,
Err(status) => {
println!("Failed to open MsQuic: 0x{:x}", status);
return;
}
};
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.
I've refactored error handling in tests in the latest commit.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4690 +/- ##
==========================================
- Coverage 86.81% 86.75% -0.06%
==========================================
Files 56 56
Lines 17361 17361
==========================================
- Hits 15072 15062 -10
- Misses 2289 2299 +10 ☔ View full report in Codecov by Sentry. |
Description
This pull request to
src/lib.rs
includes changes to improve error handling by replacing panics withResult
returns, and adds platform-specific status codes to inspect Err's content.Testing
cargo test
Documentation
No