Remove some panic possiblities. #44
Conversation
| } | ||
| } | ||
| } | ||
| impl Drop for Display { |
There was a problem hiding this comment.
This is a re implementation of functionality the rust bindings to XCB already include. see https://rtbo.github.io/rust-xcb/src/xcb/base.rs.html#595-614. The rust bindings to XCB are way easier to use, and have built-in drop implementations, making memory management a breeze.
| /// Errors that occur when trying to get display size. | ||
| #[non_exhaustive] | ||
| #[derive(Debug)] | ||
| pub enum DisplayError { |
There was a problem hiding this comment.
can you pause adding error types for a bit? I think this crate should only have one unified error type that includes all these - they're not that different, and have a fair amount of overlap.
There was a problem hiding this comment.
I agree with you. I'll try to rework errors.
Not sure it belongs in this PR though.
I prefer to have small PRs with unitary logic if possible.
There was a problem hiding this comment.
Not sure it belongs in this PR though
I agree that it doesn't belong in this PR, but it is hard to mess with error code while this PR is open (merge conflicts and such). So I suppose it's best to merge it quickly, and at the next opportunity, write a unified error type.
| if dpy_control.is_null() { | ||
| return Err(ListenError::MissingDisplayError); | ||
| } | ||
| let extension_name = CStr::from_bytes_with_nul(b"RECORD\0").unwrap(); |
There was a problem hiding this comment.
This unwrap is safe since b"RECORD\0" is const and CStr::from_bytes_with_nul is a pure function. In other words, it can be statically verified to never panic.
There was a problem hiding this comment.
How can we make sure of that ?
As far as I understand panic, unwrap and expect can lead to panics (and no more). Of course there are array[0], and division by zero errors too, but 90% of the panics should belong to the first 3 cases as far as I understand.
This is great, because I can grep those terms and find all code that is likely to panic (which is what I have done to remove them)
If we leave the original unwrap, a reader could imagine like me I guess.
Isn't the new code more explicit about the intent at least, and if the compiler can be sure it won't fail ever, then the resulting code should be the same in terms of complexity, no ?
There was a problem hiding this comment.
How can we make sure of that?
Primarily by manual reasoning - from_bytes_with_nul always works on strings that end with ascii nul and contain no other instances of nul. But also with test coverage - if it works once, it will always work, since it is a pure function with a constant argument. This is very idiomatic rust - from the book (chapter 9, section 3)
It would also be appropriate to call unwrap when you have some other logic that ensures the Result will have an Ok value, but the logic isn’t something the compiler understands.
if the compiler can be sure it won't fail ever, then the resulting code should be the same
The compiler actually isn't sure that it won't fail. See https://godbolt.org/z/pgBJam. I think the real argument for not mapping the error is that it is not truly an XRecordExtensionError, but an internal library error. A user may conclude that their Xserver is at fault if this panics, when really it is an arcane string conversion bug.
In the future, I expect from_bytes_with_nul will become a const fn, at which point the compiler will actually know that it doesn't panic, and the codegen will look the same between the safe and unsafe variant in godbolt.
There was a problem hiding this comment.
Ok, I'll move this along for now,
Rework on errors has to be done anyway to unify in following PRs.
Fixes #43.